gn: attach comments to parse tree

gn: attach comments to parse tree

(Split off of https://codereview.chromium.org/588813002)

The main change here is to restructure the ParseNode tree to include
comments. Comments are not included in the tree as normal nodes, but
instead attached subordinate to other |ParseNode|s. This is useful
for two reasons:
- it means neither the evaluator nor the main code of the parser need
  to get more complicated, as they do not need to constantly consider
  comments.
- it confers additional information about the relative location of
  of the comments to their surrounding code when (in the future)
  outputting the formatted code.

Comments are added in relation to the node they're attached to as
before, after, or suffix. This is so that formatting can keep them
attached to the correct thing when things are reflowed.

Timing data (Windows, Release):

Before:
Done. Wrote 1618 targets from 530 files in 2613ms
Done. Wrote 1618 targets from 530 files in 2952ms
Done. Wrote 1618 targets from 530 files in 3093ms

After:
Done. Wrote 1618 targets from 530 files in 2767ms
Done. Wrote 1618 targets from 530 files in 3120ms
Done. Wrote 1618 targets from 530 files in 2872ms

So, it seems within the noise at this point.

R=brettw@chromium.org
BUG=348474
Committed: https://crrev.com/be5d45107aeffccffa607908399ca0ae20512589
Cr-Commit-Position: refs/heads/master@{#296331}

[scottmg, 2014-09-22 22:12:22]

First part of gn format.

[scottmg, 2014-09-23 00:10:27]

Patchset #3 (id:40001) has been deleted

[scottmg, 2014-09-23 00:11:03]

first pass at suffix comments added.

[brettw, 2014-09-23 21:33:15]

https://codereview.chromium.org/588893006/diff/60001/tools/gn/location.cc
File tools/gn/location.cc (right):

https://codereview.chromium.org/588893006/diff/60001/tools/gn/location.cc#new...
tools/gn/location.cc:24: byte_(byte) {}
I liked the {} on separate lines.

https://codereview.chromium.org/588893006/diff/60001/tools/gn/location.h
File tools/gn/location.h (right):

https://codereview.chromium.org/588893006/diff/60001/tools/gn/location.h#newc...
tools/gn/location.h:34: int line_number_;  // -1 when unset.
Cay you document that the line and char offset are 1-based (my fault for not
doing this earlier)? I assume your byte is 0-based, so that should be mentioned
as well.

https://codereview.chromium.org/588893006/diff/60001/tools/gn/parse_tree.cc
File tools/gn/parse_tree.cc (right):

https://codereview.chromium.org/588893006/diff/60001/tools/gn/parse_tree.cc#n...
tools/gn/parse_tree.cc:55: for (const auto& i : comments_->before())
This is technically on the list of stuff "to be discussed" for the style guide.
Better nod use it yet.

https://codereview.chromium.org/588893006/diff/60001/tools/gn/parser.cc
File tools/gn/parser.cc (right):

https://codereview.chromium.org/588893006/diff/60001/tools/gn/parser.cc#newco...
tools/gn/parser.cc:546: int j = 0;
Can you give this a better name?

https://codereview.chromium.org/588893006/diff/60001/tools/gn/parser_unittest.cc
File tools/gn/parser_unittest.cc (right):

https://codereview.chromium.org/588893006/diff/60001/tools/gn/parser_unittest...
tools/gn/parser_unittest.cc:495: const char* input = "# Toplevel
comment.\n\nexecutable(\"wee\") {}\n";
Can you format your inputs on separate lines
  "# Toplevel comment.\n"
  "\n"
  "executable(\"wee\") {}\n"
It would make them much easier to follow.

https://codereview.chromium.org/588893006/diff/60001/tools/gn/tokenizer.cc
File tools/gn/tokenizer.cc (right):

https://codereview.chromium.org/588893006/diff/60001/tools/gn/tokenizer.cc#ne...
tools/gn/tokenizer.cc:130: base::TrimWhitespace(to_newline.as_string(),
base::TRIM_ALL, &trimmed);
I'd like to resolve this if possible, the tokenizer and parser is currently very
careful not to make strings.

Actually, it seems like this should be a special function that just searches
backwards and terminates if it finds a newline or a non-whitespace character.

[scottmg, 2014-09-23 22:12:01]

Patchset #4 (id:80001) has been deleted

[scottmg, 2014-09-23 22:15:37]

Thanks

https://codereview.chromium.org/588893006/diff/60001/tools/gn/location.cc
File tools/gn/location.cc (right):

https://codereview.chromium.org/588893006/diff/60001/tools/gn/location.cc#new...
tools/gn/location.cc:24: byte_(byte) {}
On 2014/09/23 21:33:15, brettw wrote:
> I liked the {} on separate lines.

Oops, clang-format, sorry. Done.

https://codereview.chromium.org/588893006/diff/60001/tools/gn/location.h
File tools/gn/location.h (right):

https://codereview.chromium.org/588893006/diff/60001/tools/gn/location.h#newc...
tools/gn/location.h:34: int line_number_;  // -1 when unset.
On 2014/09/23 21:33:15, brettw wrote:
> Cay you document that the line and char offset are 1-based (my fault for not
> doing this earlier)? I assume your byte is 0-based, so that should be
mentioned
> as well.

Done.

https://codereview.chromium.org/588893006/diff/60001/tools/gn/parse_tree.cc
File tools/gn/parse_tree.cc (right):

https://codereview.chromium.org/588893006/diff/60001/tools/gn/parse_tree.cc#n...
tools/gn/parse_tree.cc:55: for (const auto& i : comments_->before())
On 2014/09/23 21:33:15, brettw wrote:
> This is technically on the list of stuff "to be discussed" for the style
guide.
> Better nod use it yet.

Done.

https://codereview.chromium.org/588893006/diff/60001/tools/gn/parser.cc
File tools/gn/parser.cc (right):

https://codereview.chromium.org/588893006/diff/60001/tools/gn/parser.cc#newco...
tools/gn/parser.cc:546: int j = 0;
On 2014/09/23 21:33:15, brettw wrote:
> Can you give this a better name?

Done.

https://codereview.chromium.org/588893006/diff/60001/tools/gn/parser_unittest.cc
File tools/gn/parser_unittest.cc (right):

https://codereview.chromium.org/588893006/diff/60001/tools/gn/parser_unittest...
tools/gn/parser_unittest.cc:495: const char* input = "# Toplevel
comment.\n\nexecutable(\"wee\") {}\n";
On 2014/09/23 21:33:15, brettw wrote:
> Can you format your inputs on separate lines
>   "# Toplevel comment.\n"
>   "\n"
>   "executable(\"wee\") {}\n"
> It would make them much easier to follow.

Done.

https://codereview.chromium.org/588893006/diff/60001/tools/gn/tokenizer.cc
File tools/gn/tokenizer.cc (right):

https://codereview.chromium.org/588893006/diff/60001/tools/gn/tokenizer.cc#ne...
tools/gn/tokenizer.cc:130: base::TrimWhitespace(to_newline.as_string(),
base::TRIM_ALL, &trimmed);
On 2014/09/23 21:33:15, brettw wrote:
> I'd like to resolve this if possible, the tokenizer and parser is currently
very
> careful not to make strings.
> 
> Actually, it seems like this should be a special function that just searches
> backwards and terminates if it finds a newline or a non-whitespace character.

Done. (as special function, that makes more sense, I agree).

I also changed IsCurrentWhitespace (rather than over-factor it) to disallow VT
and FF, since those seem at least as silly as regular TAB.

[brettw, 2014-09-23 22:32:41]

LGTM. If you didn't notice, command_args.cc has some comment finding code in it
that should be replaced with this stuff.

[scottmg, 2014-09-23 22:44:36]

On 2014/09/23 22:32:41, brettw wrote:
> LGTM. If you didn't notice, command_args.cc has some comment finding code in
it
> that should be replaced with this stuff.

Ah, I meant to look at how it worked but I forgot. I'll switch it over.

[scottmg, 2014-09-23 22:44:39]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2014-09-23 22:45:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/588893006/100001

[commit-bot: I haz the power, 2014-09-23 23:44:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-23 23:44:57]

Try jobs failed on following builders:
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[scottmg, 2014-09-23 23:49:05]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2014-09-23 23:50:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/588893006/120001

[commit-bot: I haz the power, 2014-09-24 00:04:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-24 00:04:42]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[scottmg, 2014-09-24 00:06:50]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2014-09-24 00:07:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/588893006/120001

[commit-bot: I haz the power, 2014-09-24 00:12:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-24 00:12:45]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[scottmg, 2014-09-24 00:33:14]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2014-09-24 00:35:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/588893006/120001

[commit-bot: I haz the power, 2014-09-24 00:40:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-24 00:40:09]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[scottmg, 2014-09-24 02:18:03]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2014-09-24 02:18:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/588893006/120001

[commit-bot: I haz the power, 2014-09-24 02:29:19]

Committed patchset #5 (id:120001) as 5a1103979fdb1c690720cca88e0f855802421839

[commit-bot: I haz the power, 2014-09-24 02:29:55]

Patchset 5 (id:??) landed as
https://crrev.com/be5d45107aeffccffa607908399ca0ae20512589
Cr-Commit-Position: refs/heads/master@{#296331}