update DDC and analyzer cli preprocessArgs

update DDC and analyzer cli preprocessArgs

* refactor Analyzer CLI and DDC to use common preprocessArgs method
* throw clear exception if trailing @file not found or cannot be read
* update preprocessArgs method to always return new list

R=brianwilkerson@google.com, vsm@google.com

Committed: https://github.com/dart-lang/sdk/commit/5aa30397d72307bf93cd945b5d322db71795531a

[danrubel, 2016-12-13 18:34:39]

danrubel@google.com changed reviewers:
+ brianwilkerson@google.com, vsm@google.com

[Brian Wilkerson, 2016-12-13 18:48:21]

https://codereview.chromium.org/2572813002/diff/20001/pkg/analyzer/lib/src/co...
File pkg/analyzer/lib/src/command_line/arguments.dart (right):

https://codereview.chromium.org/2572813002/diff/20001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/command_line/arguments.dart:236: args = new
List.from(args);
Is there a reason for creating a copy of the args if they don't change?

https://codereview.chromium.org/2572813002/diff/20001/pkg/analyzer_cli/lib/sr...
File pkg/analyzer_cli/lib/src/options.dart (right):

https://codereview.chromium.org/2572813002/diff/20001/pkg/analyzer_cli/lib/sr...
pkg/analyzer_cli/lib/src/options.dart:284: bool hide = !verbose;
Did these lines accidentally get duplicated?

[danrubel, 2016-12-13 18:50:09]

https://codereview.chromium.org/2572813002/diff/20001/pkg/analyzer/lib/src/co...
File pkg/analyzer/lib/src/command_line/arguments.dart (right):

https://codereview.chromium.org/2572813002/diff/20001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/command_line/arguments.dart:236: args = new
List.from(args);
On 2016/12/13 18:48:20, Brian Wilkerson wrote:
> Is there a reason for creating a copy of the args if they don't change?

Yes. DDC expects that.

https://codereview.chromium.org/2572813002/diff/20001/pkg/analyzer_cli/lib/sr...
File pkg/analyzer_cli/lib/src/options.dart (right):

https://codereview.chromium.org/2572813002/diff/20001/pkg/analyzer_cli/lib/sr...
pkg/analyzer_cli/lib/src/options.dart:284: bool hide = !verbose;
On 2016/12/13 18:48:20, Brian Wilkerson wrote:
> Did these lines accidentally get duplicated?

Yup. Missed the duplication when I merged. Removed.

[Brian Wilkerson, 2016-12-13 19:02:16]

lgtm

https://codereview.chromium.org/2572813002/diff/20001/pkg/analyzer/lib/src/co...
File pkg/analyzer/lib/src/command_line/arguments.dart (right):

https://codereview.chromium.org/2572813002/diff/20001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/command_line/arguments.dart:236: args = new
List.from(args);
I was looking for an answer to the question of "why is this a necessary
invariant for this method?". I'm guessing that the next level up (the answer to
"why does DDC expect that") is "because they modify it", but I don't understand
why they would modify the list.

At some level it doesn't really matter because this is a one-time operation, so
copying the list needlessly isn't going to have any measurable impact on
performance, but I'm curious to understand the rationale.

That said, it seems like the reason should be given in the comment so that
someone doesn't break them later by deciding that we don't need to copy the
list.

https://codereview.chromium.org/2572813002/diff/20001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/command_line/arguments.dart:252: throw new
Exception('Failed to read file specified by $lastArg : $e');
Also, it isn't clear to me that it helps to wrap the exception. Why not just let
it be thrown (if we're not going to ignore it).

[danrubel, 2016-12-13 19:09:13]

https://codereview.chromium.org/2572813002/diff/20001/pkg/analyzer/lib/src/co...
File pkg/analyzer/lib/src/command_line/arguments.dart (right):

https://codereview.chromium.org/2572813002/diff/20001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/command_line/arguments.dart:236: args = new
List.from(args);
On 2016/12/13 19:02:16, Brian Wilkerson wrote:
> I was looking for an answer to the question of "why is this a necessary
> invariant for this method?". I'm guessing that the next level up (the answer
to
> "why does DDC expect that") is "because they modify it", but I don't
understand
> why they would modify the list.
> 
> At some level it doesn't really matter because this is a one-time operation,
so
> copying the list needlessly isn't going to have any measurable impact on
> performance, but I'm curious to understand the rationale.
> 
> That said, it seems like the reason should be given in the comment so that
> someone doesn't break them later by deciding that we don't need to copy the
> list.

Yes, they modify it :-)

I can revert this code change so that it does not copy the list and instead copy
the list in the DDC command line processing. That would push the invariant into
the code that uses it.

Which would you prefer?

https://codereview.chromium.org/2572813002/diff/20001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/command_line/arguments.dart:252: throw new
Exception('Failed to read file specified by $lastArg : $e');
On 2016/12/13 19:02:16, Brian Wilkerson wrote:
> Also, it isn't clear to me that it helps to wrap the exception. Why not just
let
> it be thrown (if we're not going to ignore it).

I wanted it to be clear that the @filePath was the cause of the file exception.
It was not obvious to me that the raw file exception would communicate that
clearly. The path in the FileException might differ from the @filePath argument
specified by the user depending upon what is returned by the resource provider.

[vsm, 2016-12-13 19:14:33]

lgtm

https://codereview.chromium.org/2572813002/diff/20001/pkg/analyzer/lib/src/co...
File pkg/analyzer/lib/src/command_line/arguments.dart (right):

https://codereview.chromium.org/2572813002/diff/20001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/command_line/arguments.dart:236: args = new
List.from(args);
On 2016/12/13 19:09:13, danrubel wrote:
> On 2016/12/13 19:02:16, Brian Wilkerson wrote:
> > I was looking for an answer to the question of "why is this a necessary
> > invariant for this method?". I'm guessing that the next level up (the answer
> to
> > "why does DDC expect that") is "because they modify it", but I don't
> understand
> > why they would modify the list.
> > 
> > At some level it doesn't really matter because this is a one-time operation,
> so
> > copying the list needlessly isn't going to have any measurable impact on
> > performance, but I'm curious to understand the rationale.
> > 
> > That said, it seems like the reason should be given in the comment so that
> > someone doesn't break them later by deciding that we don't need to copy the
> > list.
> 
> Yes, they modify it :-)
> 
> I can revert this code change so that it does not copy the list and instead
copy
> the list in the DDC command line processing. That would push the invariant
into
> the code that uses it.
> 
> Which would you prefer?

Either is fine by me.

[Brian Wilkerson, 2016-12-13 19:18:33]

https://codereview.chromium.org/2572813002/diff/20001/pkg/analyzer/lib/src/co...
File pkg/analyzer/lib/src/command_line/arguments.dart (right):

https://codereview.chromium.org/2572813002/diff/20001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/command_line/arguments.dart:236: args = new
List.from(args);
Modifying the list seems strange, but given that, I guess I don't have a strong
preference. Not modifying the list would mean that DDC would sometimes end up
copying the list twice; copying the list means that CLI will sometimes copy the
list when it doesn't need to. Unfortunate either way, but not a big deal.

https://codereview.chromium.org/2572813002/diff/20001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/command_line/arguments.dart:252: throw new
Exception('Failed to read file specified by $lastArg : $e');
Ah. Makes sense.

[danrubel, 2016-12-13 19:37:28]

Description was changed from

==========
update DDC and analyzer cli preprocessArgs

* refactor Analyzer CLI and DDC to use common preprocessArgs method
* throw clear exception if trailing @file not found or cannot be read
* update preprocessArgs method to always return new list
==========

to

==========
update DDC and analyzer cli preprocessArgs

* refactor Analyzer CLI and DDC to use common preprocessArgs method
* throw clear exception if trailing @file not found or cannot be read
* update preprocessArgs method to always return new list

R=brianwilkerson@google.com, vsm@google.com

Committed:
https://github.com/dart-lang/sdk/commit/5aa30397d72307bf93cd945b5d322db71795531a
==========

[danrubel, 2016-12-13 19:37:29]

Committed patchset #4 (id:60001) manually as
5aa30397d72307bf93cd945b5d322db71795531a (presubmit successful).