Implement line and column numbers.

Implement line and column numbers.

R=karlklose@google.com

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

[ahe, 2017-02-17 21:36:22]

Patchset #1 (id:1) has been deleted

[ahe, 2017-02-17 21:44:41]

ahe@google.com changed reviewers:
+ karlklose@google.com

[ahe, 2017-02-17 21:44:42]

This information has been crucial to implementing patch files.

On a related note: I'm still working on patch files, and I'm now able to compile
all the patch files without crashing. There's still a ton of warning about stuff
that isn't found, and I haven't even attempted running the code I generate.

To be clear: this CL contains no changes to reading patch files, only improved
diagnostics.

https://codereview.chromium.org/2704753002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart (right):

https://codereview.chromium.org/2704753002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:60: [Uri uri])
@Paul: what is your preference here? I've made the argument mandatory in
BodyBuilder as it is important that you provide this for parts as their uri is
different from the library's.

[karlklose, 2017-02-20 08:06:20]

LGTM

https://codereview.chromium.org/2704753002/diff/40001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/builder/builder.dart (right):

https://codereview.chromium.org/2704753002/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/builder/builder.dart:133: nit(library.fileUri, -1,
"'$name' is imported from both "
Consider defining a constant (like NO_COLUMN_INFORMATION) for -1 and use it
instead.

https://codereview.chromium.org/2704753002/diff/40001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/compiler_command_line.dart (right):

https://codereview.chromium.org/2704753002/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/compiler_command_line.dart:168: --fatal=errors
Why not three different flags? Does 'fatal=warnings' imply 'fatal=errors'? If
no, it feels a bit strange to have a '--option=value' pair repeated for the same
option. If yes, please add documentation stating that.

https://codereview.chromium.org/2704753002/diff/40001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/compiler_context.dart (right):

https://codereview.chromium.org/2704753002/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/compiler_context.dart:34: dynamic f(CompilerContext
c)) {
Can you come up with a more descriptive name for 'f'?

https://codereview.chromium.org/2704753002/diff/40001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/kernel_function_type_alias_builder.dart
(right):

https://codereview.chromium.org/2704753002/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_function_type_alias_builder.dart:51:
warning(parent.uri, -1, "Cyclic typedef: '$name'.");
Not sure there should be a period at the end of this message.

[ahe, 2017-02-20 08:43:27]

Patchset #3 (id:60001) has been deleted

[ahe, 2017-02-20 08:47:03]

Thank you, Karl!

I think there's a few open questions below, and I'll be happy to iterate on this
in a future CL, but I think I might as well land the CL now.

https://codereview.chromium.org/2704753002/diff/40001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/builder/builder.dart (right):

https://codereview.chromium.org/2704753002/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/builder/builder.dart:133: nit(library.fileUri, -1,
"'$name' is imported from both "
On 2017/02/20 08:06:19, karlklose wrote:
> Consider defining a constant (like NO_COLUMN_INFORMATION) for -1 and use it
> instead.

Normally, I do prefer naming constants, but it feels like -1 is so universally
recognized that it wouldn't add much value. Do you have any strong preferences
on this?

https://codereview.chromium.org/2704753002/diff/40001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/compiler_command_line.dart (right):

https://codereview.chromium.org/2704753002/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/compiler_command_line.dart:168: --fatal=errors
On 2017/02/20 08:06:19, karlklose wrote:
> Why not three different flags? Does 'fatal=warnings' imply 'fatal=errors'? If
> no, it feels a bit strange to have a '--option=value' pair repeated for the
same
> option. If yes, please add documentation stating that.

I couldn't find a way to describe it nicely before, but I've updated the
documentation.

https://codereview.chromium.org/2704753002/diff/40001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/compiler_context.dart (right):

https://codereview.chromium.org/2704753002/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/compiler_context.dart:34: dynamic f(CompilerContext
c)) {
On 2017/02/20 08:06:19, karlklose wrote:
> Can you come up with a more descriptive name for 'f'?

I think the problem was lack of documentation, so I added some.

https://codereview.chromium.org/2704753002/diff/40001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/kernel_function_type_alias_builder.dart
(right):

https://codereview.chromium.org/2704753002/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_function_type_alias_builder.dart:51:
warning(parent.uri, -1, "Cyclic typedef: '$name'.");
On 2017/02/20 08:06:19, karlklose wrote:
> Not sure there should be a period at the end of this message.

Why not? Grammatically, I think it should be:

"Cyclic typedef: '$name.'"

See
http://www.quickanddirtytips.com/education/grammar/how-to-use-quotation-marks

However, I think including the period with name would be confusing, so I put it
at the end.

Would it be better if I changed the sentence to:

"The typedef '$name' has a reference to itself."

[karlklose, 2017-02-20 08:52:35]

https://codereview.chromium.org/2704753002/diff/40001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/kernel_function_type_alias_builder.dart
(right):

https://codereview.chromium.org/2704753002/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_function_type_alias_builder.dart:51:
warning(parent.uri, -1, "Cyclic typedef: '$name'.");
On 2017/02/20 08:47:02, ahe wrote:
> On 2017/02/20 08:06:19, karlklose wrote:
> > Not sure there should be a period at the end of this message.
> 
> Why not? Grammatically, I think it should be:
> 
> "Cyclic typedef: '$name.'"
> 
> See
> http://www.quickanddirtytips.com/education/grammar/how-to-use-quotation-marks
> 
This is not about the placement of '.' in combination with quotation, but the
fact that this message is not a complete sentence and thus should not be
punctuated at all.

> However, I think including the period with name would be confusing, so I put
it
> at the end.
> 
> Would it be better if I changed the sentence to:
> 
> "The typedef '$name' has a reference to itself."

I would prefer this version.

[ahe, 2017-02-20 09:04:19]

Description was changed from

==========
Implement line and column numbers.
==========

to

==========
Implement line and column numbers.

R=karlklose@google.com

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

[ahe, 2017-02-20 09:04:20]

Committed patchset #4 (id:100001) manually as
d32f47b293d9b64978d643e93dfd10aa84e0443c (presubmit successful).

[ahe, 2017-02-20 09:05:50]

https://codereview.chromium.org/2704753002/diff/40001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/kernel_function_type_alias_builder.dart
(right):

https://codereview.chromium.org/2704753002/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_function_type_alias_builder.dart:51:
warning(parent.uri, -1, "Cyclic typedef: '$name'.");
On 2017/02/20 08:52:35, karlklose wrote:
> On 2017/02/20 08:47:02, ahe wrote:
> > On 2017/02/20 08:06:19, karlklose wrote:
> > > Not sure there should be a period at the end of this message.
> > 
> > Why not? Grammatically, I think it should be:
> > 
> > "Cyclic typedef: '$name.'"
> > 
> > See
> >
http://www.quickanddirtytips.com/education/grammar/how-to-use-quotation-marks
> > 
> This is not about the placement of '.' in combination with quotation, but the
> fact that this message is not a complete sentence and thus should not be
> punctuated at all.
> 
> > However, I think including the period with name would be confusing, so I put
> it
> > at the end.
> > 
> > Would it be better if I changed the sentence to:
> > 
> > "The typedef '$name' has a reference to itself."
> 
> I would prefer this version.

That's a good point, please don't hesitate to complain about incomplete
sentences in the future.