Move Location factories and RefactoringProblemSeverity.max into the generated classes.

Move Location factories and RefactoringProblemSeverity.max into the generated classes.

R=brianwilkerson@google.com, paulberry@google.com
BUG=

Committed: https://code.google.com/p/dart/source/detail?r=39499

[Brian Wilkerson, 2014-08-22 16:32:17]

https://codereview.chromium.org/462403005/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/generated_protocol.dart (right):

https://codereview.chromium.org/462403005/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/generated_protocol.dart:7601: static
RefactoringProblemSeverity max(RefactoringProblemSeverity a,
RefactoringProblemSeverity b) =>
I guess I wasn't clear. I think this should be an instance method so that
instead of writing

  RefactoringProblemSeverity.max(first, second)

or

  maxRefactoringProblemSeverity(first, second)

we can write

  first.max(second)

The body of _maxRefactoringProblemSeverity could then be inlined.

(Similar comment applies for the constructors. What's the value of having a
factory constructor that just calls out to a top-level function?)

[scheglov, 2014-08-22 16:43:02]

https://codereview.chromium.org/462403005/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/generated_protocol.dart (right):

https://codereview.chromium.org/462403005/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/generated_protocol.dart:7601: static
RefactoringProblemSeverity max(RefactoringProblemSeverity a,
RefactoringProblemSeverity b) =>
On 2014/08/22 16:32:17, Brian Wilkerson wrote:
> I guess I wasn't clear. I think this should be an instance method so that
> instead of writing
> 
>   RefactoringProblemSeverity.max(first, second)
> 
> or
> 
>   maxRefactoringProblemSeverity(first, second)
> 
> we can write
> 
>   first.max(second)

  Sometimes both "first" and "second" are null.
  It works fine for RefactoringStatus.addError() for example, because we set
"second".
  But in RefactoringStatus.addStatus(other) both severity of the target
RefactoringStatus and "other" may be null.
  Of course we can check for null in clients, but why not make it an API?

  Alternatively we need the RefactoringProblemSeverity.OK value which is used
instead of null.

> 
> The body of _maxRefactoringProblemSeverity could then be inlined.
> 
> (Similar comment applies for the constructors. What's the value of having a
> factory constructor that just calls out to a top-level function?)

  I follow the existing style, which, as I understand, allows us to generate
minimal code using string literals and write actual code using IDE with all its
goodies like completion, refactorings, etc.

[Paul Berry, 2014-08-22 16:46:59]

lgtm

https://codereview.chromium.org/462403005/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/generated_protocol.dart (right):

https://codereview.chromium.org/462403005/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/generated_protocol.dart:6627: factory
Location.forElement(engine.Element element) =>
Can you help me understand the motivation for this name change?  To me, "new
Location.fromElement(e)" means "take the information in element e, and create a
new Location object based on it".  Which is I think what this method does.

Whereas "new Location.forElement(e)" confuses me.  It sounds like it means
"create a Location which can be used by element e", which doesn't really make
sense.

Similar comment applies to the "forMatch", "forNode", and "forUnit" factories
below.

Not a huge deal, though, so it's your call.

[scheglov, 2014-08-22 17:14:54]

https://codereview.chromium.org/462403005/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/generated_protocol.dart (right):

https://codereview.chromium.org/462403005/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/generated_protocol.dart:6627: factory
Location.forElement(engine.Element element) =>
On 2014/08/22 16:46:59, Paul Berry wrote:
> Can you help me understand the motivation for this name change?  To me, "new
> Location.fromElement(e)" means "take the information in element e, and create
a
> new Location object based on it".  Which is I think what this method does.
> 
> Whereas "new Location.forElement(e)" confuses me.  It sounds like it means
> "create a Location which can be used by element e", which doesn't really make
> sense.
> 
> Similar comment applies to the "forMatch", "forNode", and "forUnit" factories
> below.
> 
> Not a huge deal, though, so it's your call.

You're right.
I will rename these factories/functions to "fromX".

[Brian Wilkerson, 2014-08-22 17:19:12]

LGTM

[scheglov, 2014-08-22 17:27:30]

Committed patchset #2 manually as 39499 (presubmit successful).