Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2790563003:
Use more accurate matching in fix_gn_headers.py (Closed)
DescriptionUse more accurate matching in fix_gn_headers.py
- Exclude "git grep" matches that already have header files.
- More accurate "git grep" pattern.
- Allow interactive picking which matches to handle.
BUG=661774
Review-Url: https://codereview.chromium.org/2790563003
Cr-Commit-Position: refs/heads/master@{#461317}
Committed: https://chromium.googlesource.com/chromium/src/+/eb936d60b77cbc82876bcd23e5ad7fc09a7646b1
Patch Set 1 #
Total comments: 6
Patch Set 2 : comment #Messages
Total messages: 15 (6 generated)
Patchset #1 (id:1) has been deleted
wychen@chromium.org changed reviewers: + thakis@chromium.org
PTAL https://codereview.chromium.org/2790563003/diff/20001/build/fix_gn_headers.py File build/fix_gn_headers.py (right): https://codereview.chromium.org/2790563003/diff/20001/build/fix_gn_headers.py... build/fix_gn_headers.py:20: def GitGrep(pattern): Locally I use joblib.Memory to decorate this, so I extracted it. Invalidation of the memorization is manual though.
lgtm https://codereview.chromium.org/2790563003/diff/20001/build/fix_gn_headers.py File build/fix_gn_headers.py (right): https://codereview.chromium.org/2790563003/diff/20001/build/fix_gn_headers.py... build/fix_gn_headers.py:28: def ValidMatches(basename, cc, grep_lines): function-level comment summarizing what this does https://codereview.chromium.org/2790563003/diff/20001/build/fix_gn_headers.py... build/fix_gn_headers.py:63: out, returncode = GitGrep('(/|")' + cc + '"') nit: r'\b' instead of '(/|")'
https://codereview.chromium.org/2790563003/diff/20001/build/fix_gn_headers.py File build/fix_gn_headers.py (right): https://codereview.chromium.org/2790563003/diff/20001/build/fix_gn_headers.py... build/fix_gn_headers.py:63: out, returncode = GitGrep('(/|")' + cc + '"') On 2017/03/31 16:50:57, Nico (afk until Tue Apr 4) wrote: > nit: r'\b' instead of '(/|")' '\b' is already there, but it's not enough. For example, "util.h" would match "some-util.h". Adding '/|"' makes it more accurate.
https://codereview.chromium.org/2790563003/diff/20001/build/fix_gn_headers.py File build/fix_gn_headers.py (right): https://codereview.chromium.org/2790563003/diff/20001/build/fix_gn_headers.py... build/fix_gn_headers.py:63: out, returncode = GitGrep('(/|")' + cc + '"') On 2017/03/31 16:56:56, wychen wrote: > On 2017/03/31 16:50:57, Nico (afk until Tue Apr 4) wrote: > > nit: r'\b' instead of '(/|")' > > '\b' is already there, but it's not enough. For example, "util.h" would match > "some-util.h". > Adding '/|"' makes it more accurate. Ah. using sep might be better since we might want to run this on Windows machine.
On 2017/03/31 16:58:09, wychen wrote: > https://codereview.chromium.org/2790563003/diff/20001/build/fix_gn_headers.py > File build/fix_gn_headers.py (right): > > https://codereview.chromium.org/2790563003/diff/20001/build/fix_gn_headers.py... > build/fix_gn_headers.py:63: out, returncode = GitGrep('(/|")' + cc + '"') > On 2017/03/31 16:56:56, wychen wrote: > > On 2017/03/31 16:50:57, Nico (afk until Tue Apr 4) wrote: > > > nit: r'\b' instead of '(/|")' > > > > '\b' is already there, but it's not enough. For example, "util.h" would match > > "some-util.h". > > Adding '/|"' makes it more accurate. > > Ah. using sep might be better since we might want to run this on Windows > machine. You're grepping through .gn files. They always use /.
On 2017/03/31 17:19:32, Nico (afk until Tue Apr 4) wrote: > On 2017/03/31 16:58:09, wychen wrote: > > https://codereview.chromium.org/2790563003/diff/20001/build/fix_gn_headers.py > > File build/fix_gn_headers.py (right): > > > > > https://codereview.chromium.org/2790563003/diff/20001/build/fix_gn_headers.py... > > build/fix_gn_headers.py:63: out, returncode = GitGrep('(/|")' + cc + '"') > > On 2017/03/31 16:56:56, wychen wrote: > > > On 2017/03/31 16:50:57, Nico (afk until Tue Apr 4) wrote: > > > > nit: r'\b' instead of '(/|")' > > > > > > '\b' is already there, but it's not enough. For example, "util.h" would > match > > > "some-util.h". > > > Adding '/|"' makes it more accurate. > > > > Ah. using sep might be better since we might want to run this on Windows > > machine. > > You're grepping through .gn files. They always use /. You're right. I wasn't thinking straight.
https://codereview.chromium.org/2790563003/diff/20001/build/fix_gn_headers.py File build/fix_gn_headers.py (right): https://codereview.chromium.org/2790563003/diff/20001/build/fix_gn_headers.py... build/fix_gn_headers.py:28: def ValidMatches(basename, cc, grep_lines): On 2017/03/31 16:50:57, Nico (afk until Tue Apr 4) wrote: > function-level comment summarizing what this does Done.
The CQ bit was checked by wychen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2790563003/#ps40001 (title: "comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491024747972610,
"parent_rev": "00b1fe65b09e25713e2ebb7c0e940be33cdfb7c7", "commit_rev":
"eb936d60b77cbc82876bcd23e5ad7fc09a7646b1"}
Message was sent while issue was closed.
Description was changed from ========== Use more accurate matching in fix_gn_headers.py - Exclude "git grep" matches that already have header files. - More accurate "git grep" pattern. - Allow interactive picking which matches to handle. BUG=661774 ========== to ========== Use more accurate matching in fix_gn_headers.py - Exclude "git grep" matches that already have header files. - More accurate "git grep" pattern. - Allow interactive picking which matches to handle. BUG=661774 Review-Url: https://codereview.chromium.org/2790563003 Cr-Commit-Position: refs/heads/master@{#461317} Committed: https://chromium.googlesource.com/chromium/src/+/eb936d60b77cbc82876bcd23e5ad... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) as https://chromium.googlesource.com/chromium/src/+/eb936d60b77cbc82876bcd23e5ad... |
