gn format: Have format sort sources

gn format: Have format sort sources

Sorts sources blocks as part of gn format. This ended up being more
complicated than I expected because of the subtlety of header comments,
and because no other formatting reorders expressions at the moment.
But, there seems to be quite a lot of unsorted lists per below, so I
guess it's a good thing to have.

Application to tree at https://codereview.chromium.org/960413003.

R=brettw@chromium.org
BUG=348474, 456014
Committed: https://crrev.com/5786a631ae64e933a24a62bf7a2d579fd4a36834
Cr-Commit-Position: refs/heads/master@{#318580}

[brettw, 2015-02-27 22:27:27]

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

https://codereview.chromium.org/962003002/diff/100001/tools/gn/command_format...
tools/gn/command_format.cc:319: if (lhs == "sources") {
No {} to match style

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

https://codereview.chromium.org/962003002/diff/100001/tools/gn/parse_tree.cc#...
tools/gn/parse_tree.cc:508: bool IsSeparatorComment(const ParseNode* node, const
ParseNode* prev) {
Seems like these standalone functions should be in an anon namespace at the top.

https://codereview.chromium.org/962003002/diff/100001/tools/gn/parse_tree.cc#...
tools/gn/parse_tree.cc:518: std::vector<Range> GetSortRanges(std::vector<const
ParseNode*>& contents) {
Can you provide a function level comment of what this does?

https://codereview.chromium.org/962003002/diff/100001/tools/gn/parse_tree.cc#...
tools/gn/parse_tree.cc:522: for (auto it = begin; it != contents.end(); prev =
*(it++)) {
This is pretty hard to wrap one's head around. Would this be a case where just
using ints would make it nicer? If it doesn't it's OK to keep as-is.

https://codereview.chromium.org/962003002/diff/100001/tools/gn/parse_tree.cc#...
tools/gn/parse_tree.cc:533: if (it == contents.end())
This could also go away if you used ints

https://codereview.chromium.org/962003002/diff/100001/tools/gn/parse_tree.cc#...
tools/gn/parse_tree.cc:557: for (auto range : ranges) {
You couldn't do for (auto range : GetSortRanges(contents_))? Did you need a copy
or something?

https://codereview.chromium.org/962003002/diff/100001/tools/gn/token.h
File tools/gn/token.h (right):

https://codereview.chromium.org/962003002/diff/100001/tools/gn/token.h#newcode78
tools/gn/token.h:78: void set_location(Location location) { location_ =
location; }
Seems like this should go next to location()

[scottmg, 2015-02-27 23:17:44]

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

https://codereview.chromium.org/962003002/diff/100001/tools/gn/command_format...
tools/gn/command_format.cc:319: if (lhs == "sources") {
On 2015/02/27 22:27:26, brettw wrote:
> No {} to match style

Done.

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

https://codereview.chromium.org/962003002/diff/100001/tools/gn/parse_tree.cc#...
tools/gn/parse_tree.cc:508: bool IsSeparatorComment(const ParseNode* node, const
ParseNode* prev) {
On 2015/02/27 22:27:27, brettw wrote:
> Seems like these standalone functions should be in an anon namespace at the
top.

Done.

https://codereview.chromium.org/962003002/diff/100001/tools/gn/parse_tree.cc#...
tools/gn/parse_tree.cc:518: std::vector<Range> GetSortRanges(std::vector<const
ParseNode*>& contents) {
On 2015/02/27 22:27:27, brettw wrote:
> Can you provide a function level comment of what this does?

Done.

https://codereview.chromium.org/962003002/diff/100001/tools/gn/parse_tree.cc#...
tools/gn/parse_tree.cc:522: for (auto it = begin; it != contents.end(); prev =
*(it++)) {
On 2015/02/27 22:27:27, brettw wrote:
> This is pretty hard to wrap one's head around. Would this be a case where just
> using ints would make it nicer? If it doesn't it's OK to keep as-is.

Switched to size_ts. Not sure it helps, I think it's just badly written. I
fiddled with it for a too long to get it to handle all the cases in the test,
but it's very confusing.

... While adding an explanatory comment to try to rationalize the complexity
here I realized it could easily have a test. So moved this function to being a
member, and added a test for this range extraction which matches the comment.
(Using ints helped with the test.)

https://codereview.chromium.org/962003002/diff/100001/tools/gn/parse_tree.cc#...
tools/gn/parse_tree.cc:533: if (it == contents.end())
On 2015/02/27 22:27:27, brettw wrote:
> This could also go away if you used ints

Might not have been necessary either way I think. (I previously had a nested
loop in here, but it wasn't necessary). Anyway, done.

https://codereview.chromium.org/962003002/diff/100001/tools/gn/parse_tree.cc#...
tools/gn/parse_tree.cc:557: for (auto range : ranges) {
On 2015/02/27 22:27:27, brettw wrote:
> You couldn't do for (auto range : GetSortRanges(contents_))? Did you need a
copy
> or something?

Done.

https://codereview.chromium.org/962003002/diff/100001/tools/gn/token.h
File tools/gn/token.h (right):

https://codereview.chromium.org/962003002/diff/100001/tools/gn/token.h#newcode78
tools/gn/token.h:78: void set_location(Location location) { location_ =
location; }
On 2015/02/27 22:27:27, brettw wrote:
> Seems like this should go next to location()

Done.

[brettw, 2015-02-28 00:34:08]

lgtm

[scottmg, 2015-02-28 00:41:24]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2015-02-28 00:42:07]

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

[scottmg, 2015-02-28 01:34:01]

The CQ bit was checked by scottmg@chromium.org

[scottmg, 2015-02-28 01:34:02]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/962003002/#ps140001
(title: "unsigned numbers in test")

[commit-bot: I haz the power, 2015-02-28 01:35:11]

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

[commit-bot: I haz the power, 2015-02-28 02:47:38]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-02-28 02:48:15]

Patchset 8 (id:??) landed as
https://crrev.com/5786a631ae64e933a24a62bf7a2d579fd4a36834
Cr-Commit-Position: refs/heads/master@{#318580}