Issue 418203002: Implement more fixes.

Issue 418203002: Implement more fixes. (Closed)

Created:
6 years, 5 months ago by scheglov

Modified:
6 years, 5 months ago

Reviewers:
Paul Berry

CC:
reviews_dartlang.org

Base URL:
https://dart.googlecode.com/svn/branches/bleeding_edge/dart

Visibility:
Public.

More Reviews

Description

Implement more fixes. R=paulberry@google.com BUG= Committed: https://code.google.com/p/dart/source/detail?r=38571

Patch Set 2 : Fixes for review comments #

Created: 6 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+1448 lines, -521 lines)			Patch
M	pkg/analysis_server/lib/src/edit/edit_domain.dart	View		1 chunk	+1 line, -1 line	0 comments	Download
M	pkg/analysis_server/test/edit/fix_test.dart	View		1 chunk	+1 line, -1 line	0 comments	Download
M	pkg/analysis_services/lib/correction/change.dart	View		1 chunk	+5 lines, -0 lines	0 comments	Download
M	pkg/analysis_services/lib/correction/fix.dart	View		2 chunks	+5 lines, -2 lines	0 comments	Download
M	pkg/analysis_services/lib/src/correction/fix.dart	View	1	27 chunks	+697 lines, -503 lines	0 comments	Download
M	pkg/analysis_services/lib/src/correction/source_buffer.dart	View	1	1 chunk	+4 lines, -0 lines	0 comments	Download
M	pkg/analysis_services/lib/src/correction/strings.dart	View		1 chunk	+13 lines, -0 lines	0 comments	Download
M	pkg/analysis_services/lib/src/correction/util.dart	View	1	7 chunks	+133 lines, -12 lines	0 comments	Download
M	pkg/analysis_services/test/correction/fix_test.dart	View	1	5 chunks	+589 lines, -2 lines	0 comments	Download

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

Paul Berry

lgtm https://codereview.chromium.org/418203002/diff/1/pkg/analysis_services/lib/src/correction/fix.dart File pkg/analysis_services/lib/src/correction/fix.dart (right): https://codereview.chromium.org/418203002/diff/1/pkg/analysis_services/lib/src/correction/fix.dart#newcode376 pkg/analysis_services/lib/src/correction/fix.dart:376: String eol = utils.endOfLine; With my suggestion about ...

6 years, 5 months ago (2014-07-25 00:28:46 UTC) #2

scheglov

https://codereview.chromium.org/418203002/diff/1/pkg/analysis_services/lib/src/correction/fix.dart File pkg/analysis_services/lib/src/correction/fix.dart (right): https://codereview.chromium.org/418203002/diff/1/pkg/analysis_services/lib/src/correction/fix.dart#newcode376 pkg/analysis_services/lib/src/correction/fix.dart:376: String eol = utils.endOfLine; On 2014/07/25 00:28:46, Paul Berry ...

6 years, 5 months ago (2014-07-25 03:20:52 UTC) #3

https://codereview.chromium.org/418203002/diff/1/pkg/analysis_services/lib/sr...
File pkg/analysis_services/lib/src/correction/fix.dart (right):

https://codereview.chromium.org/418203002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/correction/fix.dart:376: String eol =
utils.endOfLine;
On 2014/07/25 00:28:46, Paul Berry wrote:
> With my suggestion about eol below, this line could be eliminated.

Done.

https://codereview.chromium.org/418203002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/correction/fix.dart:386: sb.startPosition('NAME');
On 2014/07/25 00:28:45, Paul Berry wrote:
> Are the strings passed to startPosition things that the client should
recognize?
>  If so, they probably they should be in constants.dart rather than hardcoded
> here.

These strings are IDs of linked position groups.
They are not used by clients.

https://codereview.chromium.org/418203002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/correction/fix.dart:654: } else if (isMethod ||
isSetter) {
On 2014/07/25 00:28:45, Paul Berry wrote:
> Any reason this couldn't just be "} else {"?

Done.

https://codereview.chromium.org/418203002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/correction/fix.dart:661: if (isMethod) {
On 2014/07/25 00:28:45, Paul Berry wrote:
> This if test is unnecessary, since we do the same thing in both cases.

Done.

https://codereview.chromium.org/418203002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/correction/fix.dart:1224: if (typeSource !=
"dynamic") {
On 2014/07/25 00:28:46, Paul Berry wrote:
> It seems weird that we use a string comparison here, but in _appendType() we
use
> type.isDynamic.  I'd prefer to use type.isDynamic consistently.
> 
> Or, perhaps even better: share this code with _appendType() (by giving
> appendType() an optional argument which, if supplied, causes startPosition()
and
> endPosition() to be called, and gets passed to startPosition()).

Done.

https://codereview.chromium.org/418203002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/correction/fix.dart:1556: if (typeSource !=
"dynamic") {
On 2014/07/25 00:28:46, Paul Berry wrote:
> Use returnType.isDynamic here too.

Done.

https://codereview.chromium.org/418203002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/correction/fix.dart:1582: {
On 2014/07/25 00:28:45, Paul Berry wrote:
> Should we guard this with "if (!type.isDynamic)", like we do for the return
> type?

Done.
I've also added a test.

https://codereview.chromium.org/418203002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/correction/fix.dart:1598: sb.append("
{${eol}${prefix}}");
On 2014/07/25 00:28:46, Paul Berry wrote:
> Space before $eol seems unnecessary.

The space is before "{", which opens the body block.

https://codereview.chromium.org/418203002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/correction/fix.dart:1644: String eol =
utils.endOfLine;
On 2014/07/25 00:28:45, Paul Berry wrote:
> Suggestion: rather than force the caller to look up utils.endOfLine and pass
it
> in to _addProposal_createFunction(), how about creating this getter in the
> FixProcessor class?
> 
>   String get eol => utils.endOfLine;
> 
> (similar to what you've done for coreTypeBool)

Done.

https://codereview.chromium.org/418203002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/correction/fix.dart:1656: sourcePrefix =
"${prefix}${eol}";
On 2014/07/25 00:28:45, Paul Berry wrote:
> This will just insert a blank line containing whitespace, right?  It seems
like
> 
>   sourcePrefix = eol;
> 
> would be better.

Done.

https://codereview.chromium.org/418203002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/correction/fix.dart:1812: bool
_inStaticMemberContext() {
On 2014/07/25 00:28:45, Paul Berry wrote:
> This won't do the right thing if node is in a constructor field initializer. 
> E.g. with this input:
> 
> class A {
>   var x;
>   A() : x = foo();
> }
> 
> foo should be considered to be in a static member context.

Added the test and fixed.

https://codereview.chromium.org/418203002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/correction/fix.dart:1814: return
_inStaticMemberContext2(member);
On 2014/07/25 00:28:45, Paul Berry wrote:
> Rather than having two functions and forcing the caller to remember whether to
> append a "2" to the function name, how about if we just make "member" an
> optional parameter?

Done.

https://codereview.chromium.org/418203002/diff/1/pkg/analysis_services/lib/sr...
File pkg/analysis_services/lib/src/correction/util.dart (right):

https://codereview.chromium.org/418203002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/correction/util.dart:161: if ("dynamic" ==
typeSource) {
On 2014/07/25 00:28:46, Paul Berry wrote:
> Use type.isDynamic here too.

Done.

https://codereview.chromium.org/418203002/diff/1/pkg/analysis_services/test/c...
File pkg/analysis_services/test/correction/fix_test.dart (right):

https://codereview.chromium.org/418203002/diff/1/pkg/analysis_services/test/c...
pkg/analysis_services/test/correction/fix_test.dart:1036: edits =
edits.reversed.toList();
On 2014/07/25 00:28:46, Paul Berry wrote:
> What's the motivation for this change?

In order to avoid fixing offsets for Edit objects we want to apply them starting
with ones with highest offsets.

When we create missing overrides, we want to declared new members sorted by
name, so order matters.
The initial implementation generated each member separately, in separate Edits.
I'm going to change it to use a single SourceBuilder and rollback the change
here.

scheglov

6 years, 5 months ago (2014-07-25 03:21:21 UTC) #4

Message was sent while issue was closed.

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

Expand Messages | Collapse Messages