gn: start of format command

gn: start of format command

Adds `gn format` command. Currently accepts one //path/to/BUILD.gn and
attempts to reformat it (to stdout). Handles the basics, but does not
handle all constructs yet, and doesn't attempt any rewrites.

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

[scottmg, 2014-09-25 00:06:47]

Patchset #12 (id:220001) has been deleted

[scottmg, 2014-09-25 20:17:44]

As discussed, now with some additions to the main parse tree now for standalone
comments.

You can see what it does by flipping through the .gn/.golden tests.

There's two tests here that aren't run yet because they don't work properly yet
(015, 016), but it's probably a good time for you to have a look before this
gets too much bigger. I think we can figure out something that works for the
comment cases, even if it ends up being a bit ugly in the printer.

I haven't looked into maintaining parentheses in the parse tree yet (that's 016,
which will currently drop the '(' and ')' making it incorrect), but I'm hopeful
that the formatter can either be smart enough to print with a minimal, clear set
of ()s, or if necessary we can add () to the tree.

[brettw, 2014-09-25 22:11:32]

lgtm

https://codereview.chromium.org/591373002/diff/320001/tools/gn/command_format.cc
File tools/gn/command_format.cc (right):

https://codereview.chromium.org/591373002/diff/320001/tools/gn/command_format...
tools/gn/command_format.cc:24: "gn format: Format .gn file.\n"
Can you append here "(alpha, use at own risk)" or something. It's easy to see
the commands and not read the long help.

https://codereview.chromium.org/591373002/diff/320001/tools/gn/command_format...
tools/gn/command_format.cc:71: // Format a list of values using the given style.
These enums should go at the top of the "private" section.

https://codereview.chromium.org/591373002/diff/320001/tools/gn/command_format...
tools/gn/command_format.cc:81: enum { kIndentSize = 2 };
Can this be a static const int instead so it's typed?

https://codereview.chromium.org/591373002/diff/320001/tools/gn/command_format...
tools/gn/command_format.cc:107: output_ += buf;
Can this all be replaced with:
  va_list ap;
  va_start(ap, fmt);
  output_ += base::StringPrintV(fmt, ap);
  va_end(ap);

Actually, I take that back. It looks like the only thing this is used for is
printing margins and I find the usage difficult to read. Maybe add a new
function PrintMargin that creates a std::string with the right number of spaces
and prints it, and then use plain Print() for the non-margin chars?

https://codereview.chromium.org/591373002/diff/320001/tools/gn/command_format...
tools/gn/command_format.cc:163: if (stmt->comments()) {
What about "before" statement comments?

https://codereview.chromium.org/591373002/diff/320001/tools/gn/command_format...
tools/gn/command_format.cc:329: std::string* output, bool dump_tree) {
Out arg should be last.

https://codereview.chromium.org/591373002/diff/320001/tools/gn/format_test_da...
File tools/gn/format_test_data/003.golden (right):

https://codereview.chromium.org/591373002/diff/320001/tools/gn/format_test_da...
tools/gn/format_test_data/003.golden:7: deps = [ "//base" ]
I usually like source and deps on separate lines even if there is only one since
it makes it easier to add stuff. But this is fine for now, and I won't be too
sad if we decide to change the rule to make the autoformatting easier.

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

https://codereview.chromium.org/591373002/diff/320001/tools/gn/parse_tree.cc#...
tools/gn/parse_tree.cc:605: const std::string& help) const {
indenting

[scottmg, 2014-09-25 23:03:12]

Thanks

https://codereview.chromium.org/591373002/diff/320001/tools/gn/command_format.cc
File tools/gn/command_format.cc (right):

https://codereview.chromium.org/591373002/diff/320001/tools/gn/command_format...
tools/gn/command_format.cc:24: "gn format: Format .gn file.\n"
On 2014/09/25 22:11:32, brettw wrote:
> Can you append here "(alpha, use at own risk)" or something. It's easy to see
> the commands and not read the long help.

Done.

https://codereview.chromium.org/591373002/diff/320001/tools/gn/command_format...
tools/gn/command_format.cc:71: // Format a list of values using the given style.
On 2014/09/25 22:11:32, brettw wrote:
> These enums should go at the top of the "private" section.

Done.

https://codereview.chromium.org/591373002/diff/320001/tools/gn/command_format...
tools/gn/command_format.cc:81: enum { kIndentSize = 2 };
On 2014/09/25 22:11:32, brettw wrote:
> Can this be a static const int instead so it's typed?

Done.

https://codereview.chromium.org/591373002/diff/320001/tools/gn/command_format...
tools/gn/command_format.cc:107: output_ += buf;
On 2014/09/25 22:11:32, brettw wrote:
> Can this all be replaced with:
>   va_list ap;
>   va_start(ap, fmt);
>   output_ += base::StringPrintV(fmt, ap);
>   va_end(ap);
> 
> Actually, I take that back. It looks like the only thing this is used for is
> printing margins and I find the usage difficult to read. Maybe add a new
> function PrintMargin that creates a std::string with the right number of
spaces
> and prints it, and then use plain Print() for the non-margin chars?

Good point! I had other more complicated format strings before, but they got
simpler.

Done.

https://codereview.chromium.org/591373002/diff/320001/tools/gn/command_format...
tools/gn/command_format.cc:163: if (stmt->comments()) {
On 2014/09/25 22:11:32, brettw wrote:
> What about "before" statement comments?

Hmm, the asymmetry is kind of confusing. The before (and suffix) are handled
inside Expr() with before() directly printed, and suffix() queued to the next
newline (so that they go after trailing commas). But after() should follow the
newline. But since it's a general expression printer, the newline can't happen
inside, because there's only newlines for statements in blocks. So the after()s
are here.

But what about Sequence then, it should need to print the after() comments in
the same way? I don't think there's any case (at least at the moment) where
after()s are assigned in the middle of a list. They're either standalone, or
attached as before() to a list element. Added CHECK for that.

https://codereview.chromium.org/591373002/diff/320001/tools/gn/command_format...
tools/gn/command_format.cc:329: std::string* output, bool dump_tree) {
On 2014/09/25 22:11:32, brettw wrote:
> Out arg should be last.

Done.

https://codereview.chromium.org/591373002/diff/320001/tools/gn/format_test_da...
File tools/gn/format_test_data/003.golden (right):

https://codereview.chromium.org/591373002/diff/320001/tools/gn/format_test_da...
tools/gn/format_test_data/003.golden:7: deps = [ "//base" ]
On 2014/09/25 22:11:32, brettw wrote:
> I usually like source and deps on separate lines even if there is only one
since
> it makes it easier to add stuff. But this is fine for now, and I won't be too
> sad if we decide to change the rule to make the autoformatting easier.

You mean specifically just for source and deps, not all lists? Right now it
doesn't know what the lhs of the list is, but we could certainly have a pass
that walks the tree and forces things the way we like them.

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

https://codereview.chromium.org/591373002/diff/320001/tools/gn/parse_tree.cc#...
tools/gn/parse_tree.cc:605: const std::string& help) const {
On 2014/09/25 22:11:32, brettw wrote:
> indenting

Done.

[scottmg, 2014-09-25 23:04:38]

(check out those const auto&'s btw!, woohoo)

[scottmg, 2014-09-25 23:04:53]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2014-09-25 23:06:47]

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

[brettw, 2014-09-25 23:06:51]

On 2014/09/25 23:04:38, scottmg wrote:
> (check out those const auto&'s btw!, woohoo)

I know! I want to write some.

[brettw, 2014-09-25 23:06:58]

https://codereview.chromium.org/591373002/diff/320001/tools/gn/command_format.cc
File tools/gn/command_format.cc (right):

https://codereview.chromium.org/591373002/diff/320001/tools/gn/command_format...
tools/gn/command_format.cc:163: if (stmt->comments()) {
Can you explain this in a comment?

https://codereview.chromium.org/591373002/diff/320001/tools/gn/format_test_da...
File tools/gn/format_test_data/003.golden (right):

https://codereview.chromium.org/591373002/diff/320001/tools/gn/format_test_da...
tools/gn/format_test_data/003.golden:7: deps = [ "//base" ]
Yeah, in the style guide I listed these as separate cases to always use the
multiline format when assigning (but not +=. But with an autoformatter, I'm not
necessarily wedded to this, and it could be a bad idea.

[scottmg, 2014-09-25 23:10:22]

The CQ bit was unchecked by scottmg@chromium.org

[scottmg, 2014-09-25 23:13:12]

https://codereview.chromium.org/591373002/diff/320001/tools/gn/command_format.cc
File tools/gn/command_format.cc (right):

https://codereview.chromium.org/591373002/diff/320001/tools/gn/command_format...
tools/gn/command_format.cc:163: if (stmt->comments()) {
On 2014/09/25 23:06:58, brettw wrote:
> Can you explain this in a comment?

Done.

[scottmg, 2014-09-25 23:15:59]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2014-09-25 23:17:18]

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

[commit-bot: I haz the power, 2014-09-25 23:33:07]

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

[commit-bot: I haz the power, 2014-09-25 23:33:09]

Try jobs failed on following builders:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[scottmg, 2014-09-25 23:37:24]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2014-09-25 23:38:20]

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

[commit-bot: I haz the power, 2014-09-26 00:30:31]

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

[commit-bot: I haz the power, 2014-09-26 00:30:33]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[scottmg, 2014-09-26 00:36:48]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2014-09-26 00:38:55]

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

[commit-bot: I haz the power, 2014-09-26 01:17:59]

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

[commit-bot: I haz the power, 2014-09-26 01:18:00]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[scottmg, 2014-09-26 03:15:29]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2014-09-26 03:16:16]

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

[commit-bot: I haz the power, 2014-09-26 04:07:14]

Committed patchset #22 (id:420002) as 2aa2d72aab6956689836abf48bf4743411204e38

[commit-bot: I haz the power, 2014-09-26 04:07:58]

Patchset 22 (id:??) landed as
https://crrev.com/2dd93e8b9b97ad141b0a9ac6bef703926ec3b306
Cr-Commit-Position: refs/heads/master@{#296876}