Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1496653002:
Sort "deps" and "public_deps" when running the "gn format" command. (Closed)
DescriptionSort "deps" and "public_deps" when running the "gn format" command.
The "deps" and "public_deps" are sorted using the following order:
1. litteral strings
a. local targets, e.g. ":a",
b. relative targets, e.g. "a" or "../a",
c. absolute targets, e.g. "//a",
2. other values, e.g. variable reference.
In each group, values are sorted alphabetically.
BUG=554928
Committed: https://crrev.com/b502732dcdb76fae1e8c2f8ddc63a019e9a9ecdd
Cr-Commit-Position: refs/heads/master@{#362943}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Tweak order according to dpranke's comment #
Total comments: 5
Patch Set 3 : Address comments & add some test cases #
Messages
Total messages: 23 (8 generated)
sdefresne@chromium.org changed reviewers: + scottmg@chromium.org
Please take a look.
Note: CL applying the sort order is at https://codereview.chromium.org/1496653002.
Copy-n-paste fail, correct link is https://codereview.chromium.org/1497543003.
Description was changed from
==========
Sort "deps" and "public_deps" when running the "gn format" command.
The "deps" and "public_deps" are sorted using the following order:
1. litteral strings
1. relative targets, e.g. "a" or "../a",
2. local targets, e.g. ":a",
3. absolute targets, e.g. "//a",
2. other values, e.g. variable reference.
In each group, values are sorted alphabetically.
BUG=554928
==========
to
==========
Sort "deps" and "public_deps" when running the "gn format" command.
The "deps" and "public_deps" are sorted using the following order:
1. literal strings
1. relative targets, e.g. "a" or "../a",
2. local targets, e.g. ":a",
3. absolute targets, e.g. "//a",
2. other values, e.g. variable reference.
In each group, values are sorted alphabetically.
BUG=554928
==========
dpranke@chromium.org changed reviewers: + brettw@chromium.org, dpranke@chromium.org
Brett, Scott, thoughts? https://codereview.chromium.org/1496653002/diff/1/tools/gn/format_test_data/0... File tools/gn/format_test_data/063.gn (right): https://codereview.chromium.org/1496653002/diff/1/tools/gn/format_test_data/0... tools/gn/format_test_data/063.gn:9: a, This is definitely not right. I would think it should be: deps = [ ":a", "a", "//a", "//a:a", "//a/a", a, ] though I could also see a case for moving the last line (the variable) elsewhere in the ordering.
Description was changed from
==========
Sort "deps" and "public_deps" when running the "gn format" command.
The "deps" and "public_deps" are sorted using the following order:
1. literal strings
1. relative targets, e.g. "a" or "../a",
2. local targets, e.g. ":a",
3. absolute targets, e.g. "//a",
2. other values, e.g. variable reference.
In each group, values are sorted alphabetically.
BUG=554928
==========
to
==========
Sort "deps" and "public_deps" when running the "gn format" command.
The "deps" and "public_deps" are sorted using the following order:
1. litteral strings
a. local targets, e.g. ":a",
b. relative targets, e.g. "a" or "../a",
c. absolute targets, e.g. "//a",
2. other values, e.g. variable reference.
In each group, values are sorted alphabetically.
BUG=554928
==========
On 2015/12/02 at 18:53:28, dpranke wrote: > Brett, Scott, thoughts? > > https://codereview.chromium.org/1496653002/diff/1/tools/gn/format_test_data/0... > File tools/gn/format_test_data/063.gn (right): > > https://codereview.chromium.org/1496653002/diff/1/tools/gn/format_test_data/0... > tools/gn/format_test_data/063.gn:9: a, > This is definitely not right. I would think it should be: > > deps = [ > ":a", > "a", > "//a", > "//a:a", > "//a/a", > a, > ] > > though I could also see a case for moving the last line (the variable) elsewhere in the ordering. Fixed, better now?
lgtm
tfarina@chromium.org changed reviewers: + tfarina@chromium.org
https://codereview.chromium.org/1496653002/diff/20001/tools/gn/parse_tree.cc File tools/gn/parse_tree.cc (right): https://codereview.chromium.org/1496653002/diff/20001/tools/gn/parse_tree.cc#... tools/gn/parse_tree.cc:20: kDepsCategoryLocal = 0, my understanding from the style-guide is that this should be ALL_CAPS. Do you mind changing? https://codereview.chromium.org/1496653002/diff/20001/tools/gn/parse_tree.cc#... tools/gn/parse_tree.cc:26: DepsCategory GetDepsCategory(base::StringPiece deps) { is it more recommended to pass it by value rather than ref-to-const?
lgtm
lgtm https://codereview.chromium.org/1496653002/diff/20001/tools/gn/parse_tree.cc File tools/gn/parse_tree.cc (right): https://codereview.chromium.org/1496653002/diff/20001/tools/gn/parse_tree.cc#... tools/gn/parse_tree.cc:20: kDepsCategoryLocal = 0, nit; Drop the " = N" since it's just for ordering purposes and they're all default values anyway.
The CQ bit was checked by sdefresne@chromium.org
Thank you for the reviews. https://codereview.chromium.org/1496653002/diff/20001/tools/gn/parse_tree.cc File tools/gn/parse_tree.cc (right): https://codereview.chromium.org/1496653002/diff/20001/tools/gn/parse_tree.cc#... tools/gn/parse_tree.cc:20: kDepsCategoryLocal = 0, On 2015/12/02 at 22:12:12, scottmg wrote: > nit; Drop the " = N" since it's just for ordering purposes and they're all default values anyway. Done both (dropped = N, used ALL_CAPS). https://codereview.chromium.org/1496653002/diff/20001/tools/gn/parse_tree.cc#... tools/gn/parse_tree.cc:26: DepsCategory GetDepsCategory(base::StringPiece deps) { On 2015/12/02 at 20:19:32, tfarina wrote: > is it more recommended to pass it by value rather than ref-to-const? base/strings/string_piece.h recommends passing by value: // Prefer passing StringPieces by value: // void MyFunction(StringPiece arg); https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin...
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, brettw@chromium.org, scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/1496653002/#ps40001 (title: "Address comments & add some test cases")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1496653002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1496653002/40001
tfarina: will you be taking care of deployment of a new version of gn with that fixed ordering of deps? If so, could you send the followup CL https://codereview.chromium.org/1497543003/ to the CQ once this is done (I can't now because the ordering is considered broken by PRESUBMIT check). Cheers, -- Sylvain On Thu, 3 Dec 2015 at 10:30 commit-bot@chromium.org via codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1496653002/40001 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/1496653002/40001 > > > https://codereview.chromium.org/1496653002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from
==========
Sort "deps" and "public_deps" when running the "gn format" command.
The "deps" and "public_deps" are sorted using the following order:
1. litteral strings
a. local targets, e.g. ":a",
b. relative targets, e.g. "a" or "../a",
c. absolute targets, e.g. "//a",
2. other values, e.g. variable reference.
In each group, values are sorted alphabetically.
BUG=554928
==========
to
==========
Sort "deps" and "public_deps" when running the "gn format" command.
The "deps" and "public_deps" are sorted using the following order:
1. litteral strings
a. local targets, e.g. ":a",
b. relative targets, e.g. "a" or "../a",
c. absolute targets, e.g. "//a",
2. other values, e.g. variable reference.
In each group, values are sorted alphabetically.
BUG=554928
Committed: https://crrev.com/b502732dcdb76fae1e8c2f8ddc63a019e9a9ecdd
Cr-Commit-Position: refs/heads/master@{#362943}
==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/b502732dcdb76fae1e8c2f8ddc63a019e9a9ecdd Cr-Commit-Position: refs/heads/master@{#362943}
Message was sent while issue was closed.
On 2015/12/03 09:32:33, sdefresne wrote: > tfarina: will you be taking care of deployment of a new version of gn with > that fixed ordering of deps? If so, could you send the followup CL > https://codereview.chromium.org/1497543003/ to the CQ once this is done (I > can't now because the ordering is considered broken by PRESUBMIT check). > Sorry, I forgot to wait for that change to land before pushing another gn binary. Looks like it didn't make into the binary I pushed yesterday. I will have to do another push, so this change gets in. But yeah, I will look into that. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
