Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2599193002:
Split run_tool.py into run_tool.py, extract_edits.py and apply_edits.py (Closed)
|
Created:
3 years, 12 months ago by Łukasz Anforowicz Modified:
3 years, 11 months ago CC:
chromium-reviews, danakj, dsansome Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSplit run_tool.py into run_tool.py, extract_edits.py and apply_edits.py
The split will allow generation of edits on multiple configs (e.g. linux
vs windows OR rel vs dbg) and merging the edits before applying them once:
$ tools/clang/scripts/run_tool.py rewrite_to_chrome_style \
--generate-compdb --all out/rel >run_tool.linux.rel.out
$ ...
$ cat run_tool.*.out \
| tools/clang/scripts/extract_edits.py \
| tools/clang/scripts/apply_edits.py
--generate-compdb --all out/rel >run_tool.linux.rel.out
Test steps:
- tools/clang/translation_unit/test_translation_unit.py
- tools/clang/scripts/test_tool.py rewrite_to_chrome_style
- manually running run_tool | extract_edits | apply_edits pipeline
on WTF and verifying that it still builds after the rename
BUG=598138
TEST=See "Test steps" above.
Committed: https://crrev.com/f9b89e7bd49caf2c5137bd540528001d36b577bc
Cr-Commit-Position: refs/heads/master@{#440881}
Patch Set 1 #Patch Set 2 : --similarity=15 #
Total comments: 40
Patch Set 3 : Addressed CR feedback from dcheng@ and danakj@. #
Total comments: 14
Patch Set 4 : Addressed remaining nits. #
Messages
Total messages: 19 (8 generated)
lukasza@chromium.org changed reviewers: + dcheng@chromium.org
dcheng@, can you PTAL?
Description was changed from
==========
Split run_tool.py into run_tool.py, extract_edits.py and apply_edits.py
The split will allow generation of edits on multiple configs (e.g. linux
vs windows OR rel vs dbg) and merging the edits before applying them once:
$ tools/clang/scripts/run_tool.py rewrite_to_chrome_style \
--generate-compdb --all out/rel >run_tool.linux.rel.out
$ ...
$ cat run_tool.*.out \
| tools/clang/scripts/extract_edits.py \
| tools/clang/scripts/apply_edits.py
--generate-compdb --all out/rel >run_tool.linux.rel.out
Test steps:
- tools/clang/translation_unit/test_translation_unit.py
- tools/clang/scripts/test_tool.py rewrite_to_chrome_style
- manually running run_tool | extract_edits | apply_edits pipeline
BUG=598138
TEST=See "Test steps" above.
==========
to
==========
Split run_tool.py into run_tool.py, extract_edits.py and apply_edits.py
The split will allow generation of edits on multiple configs (e.g. linux
vs windows OR rel vs dbg) and merging the edits before applying them once:
$ tools/clang/scripts/run_tool.py rewrite_to_chrome_style \
--generate-compdb --all out/rel >run_tool.linux.rel.out
$ ...
$ cat run_tool.*.out \
| tools/clang/scripts/extract_edits.py \
| tools/clang/scripts/apply_edits.py
--generate-compdb --all out/rel >run_tool.linux.rel.out
Test steps:
- tools/clang/translation_unit/test_translation_unit.py
- tools/clang/scripts/test_tool.py rewrite_to_chrome_style
- manually running run_tool | extract_edits | apply_edits pipeline
on WTF and verifying that it still builds after the rename
BUG=598138
TEST=See "Test steps" above.
==========
danakj@chromium.org changed reviewers: + danakj@chromium.org
https://codereview.chromium.org/2599193002/diff/20001/docs/clang_tool_refacto... File docs/clang_tool_refactoring.md (right): https://codereview.chromium.org/2599193002/diff/20001/docs/clang_tool_refacto... docs/clang_tool_refactoring.md:141: out/Debug <path 1> <path 2> ... >/tmp/list-of-edits suggest putting debug in the output file name for good role modelling https://codereview.chromium.org/2599193002/diff/20001/docs/clang_tool_refacto... docs/clang_tool_refactoring.md:155: out/Debug base >/tmp/list-of-edits same https://codereview.chromium.org/2599193002/diff/20001/docs/clang_tool_refacto... docs/clang_tool_refactoring.md:161: cat /tmp/list-of-edits \ same https://codereview.chromium.org/2599193002/diff/20001/docs/clang_tool_refacto... docs/clang_tool_refactoring.md:163: | tools/clang/scripts/apply_edits.py out/Debug <optional paths to filter by> Why are there filter paths here and for run_tool.py, i guess this can only be a subset of those? If so maybe mention this in the text above when saying "apply the edits" https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/app... File tools/clang/scripts/apply_edits.py (right): https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/app... tools/clang/scripts/apply_edits.py:124: sys.stderr.flush() stderr doesn't need flush right? https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/ext... File tools/clang/scripts/extract_edits.py (right): https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/ext... tools/clang/scripts/extract_edits.py:50: unique_lines.add(line) jw why bother with this, it will make this tool slower, and we already have de-duping in the apply script? https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/run... File tools/clang/scripts/run_tool.py (right): https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/run... tools/clang/scripts/run_tool.py:35: source files. These tools assumme the clang tool emits the edits in the assume https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/run... tools/clang/scripts/run_tool.py:95: def _AtomicallyForwardToolOutput(stdout): name this |text| or something not |stdout|, which would be used to refer to the fd or something https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/run... tools/clang/scripts/run_tool.py:122: If status is True, then the generated edits are stored with the key "edits" needs update https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/run... tools/clang/scripts/run_tool.py:174: sys.stderr.flush() stderr doesnt need flush? https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/run... tools/clang/scripts/run_tool.py:194: sys.stderr.flush() same https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/tes... File tools/clang/scripts/test_tool.py (right): https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/tes... tools/clang/scripts/test_tool.py:51: # run_tools.py will skip them when applying replacements. run_tools.py? https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/tes... tools/clang/scripts/test_tool.py:72: '/'] # compile.db created by test_tool.py uses absolute paths. isn't the path optional? why specify it then?
LGTM once that stuff is resolved
+dsansome as FYI, since this potentially affects the code search pipeline (hopefully in a positive way)
https://codereview.chromium.org/2599193002/diff/20001/docs/clang_tool_refacto... File docs/clang_tool_refactoring.md (right): https://codereview.chromium.org/2599193002/diff/20001/docs/clang_tool_refacto... docs/clang_tool_refactoring.md:158: Finally - apply the edits as follows: Super minor nit: comma instead of a dash https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/app... File tools/clang/scripts/apply_edits.py (right): https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/app... tools/clang/scripts/apply_edits.py:5: """Applies edits generated by a clang tools that was run on Chromium code. Nit: a clang tool or clang tools https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/app... tools/clang/scripts/apply_edits.py:188: return 0 In theory, we could detect overlapping conflicting edits and return an error here. (This might have been useful for the conflicting Frame/GetFrame rewrite) https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/ext... File tools/clang/scripts/extract_edits.py (right): https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/ext... tools/clang/scripts/extract_edits.py:7: Clang tools are expected to emit edits like this: Not all clang tools, just rewriter tools, right? https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/ext... tools/clang/scripts/extract_edits.py:50: unique_lines.add(line) On 2016/12/23 15:45:06, danakj_OOO_and_slow wrote: > jw why bother with this, it will make this tool slower, and we already have > de-duping in the apply script? The apply script reads all the edits before deduping. It could be a fairly decent memory savings from dropping repeated edits in things like headers that are included a lot. https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/ext... tools/clang/scripts/extract_edits.py:51: print(line) For consistency, no () https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/run... File tools/clang/scripts/run_tool.py (right): https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/run... tools/clang/scripts/run_tool.py:95: def _AtomicallyForwardToolOutput(stdout): On 2016/12/23 15:45:06, danakj_OOO_and_slow wrote: > name this |text| or something not |stdout|, which would be used to refer to the > fd or something Rather than doing this, should we just 'return' the string and print to stdout from the dispatcher?
https://codereview.chromium.org/2599193002/diff/20001/docs/clang_tool_refacto... File docs/clang_tool_refactoring.md (right): https://codereview.chromium.org/2599193002/diff/20001/docs/clang_tool_refacto... docs/clang_tool_refactoring.md:141: out/Debug <path 1> <path 2> ... >/tmp/list-of-edits On 2016/12/23 15:45:06, danakj_OOO_and_slow wrote: > suggest putting debug in the output file name for good role modelling Done. https://codereview.chromium.org/2599193002/diff/20001/docs/clang_tool_refacto... docs/clang_tool_refactoring.md:155: out/Debug base >/tmp/list-of-edits On 2016/12/23 15:45:06, danakj_OOO_and_slow wrote: > same Done. https://codereview.chromium.org/2599193002/diff/20001/docs/clang_tool_refacto... docs/clang_tool_refactoring.md:158: Finally - apply the edits as follows: On 2016/12/27 07:30:14, dcheng wrote: > Super minor nit: comma instead of a dash Done. https://codereview.chromium.org/2599193002/diff/20001/docs/clang_tool_refacto... docs/clang_tool_refactoring.md:161: cat /tmp/list-of-edits \ On 2016/12/23 15:45:06, danakj_OOO_and_slow wrote: > same Done. https://codereview.chromium.org/2599193002/diff/20001/docs/clang_tool_refacto... docs/clang_tool_refactoring.md:163: | tools/clang/scripts/apply_edits.py out/Debug <optional paths to filter by> On 2016/12/23 15:45:06, danakj_OOO_and_slow wrote: > Why are there filter paths here and for run_tool.py, i guess this can only be a > subset of those? If so maybe mention this in the text above when saying "apply > the edits" I tried to explain this better in the newest patchset. 1. filters passed to run_tool.py control which source files the clang tool is run against. the tool can emit edits that apply to *any* files anywhere (run_tool.py doesn't look at the output of the clang tool at all - the tool can legitimately produce edits somewhere "outside" [e.g. when processing a .cc file under //net it can generate edits for an included header file under //base]). 2. filters passed to apply_edits.py controls which files get edited https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/app... File tools/clang/scripts/apply_edits.py (right): https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/app... tools/clang/scripts/apply_edits.py:5: """Applies edits generated by a clang tools that was run on Chromium code. On 2016/12/27 07:30:14, dcheng wrote: > Nit: a clang tool or clang tools Done. https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/app... tools/clang/scripts/apply_edits.py:124: sys.stderr.flush() On 2016/12/23 15:45:06, danakj_OOO_and_slow wrote: > stderr doesn't need flush right? Done. Things (e.g. updating the status line) seem to work without it. I don't know why it was here before (i.e. see line 214). https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/app... tools/clang/scripts/apply_edits.py:188: return 0 On 2016/12/27 07:30:14, dcheng wrote: > In theory, we could detect overlapping conflicting edits and return an error > here. > > (This might have been useful for the conflicting Frame/GetFrame rewrite) Done. Hopefully what I did is "Good Enough (tm)" for now. I only detect conflicts if offset and length are the same but replacements are different. https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/ext... File tools/clang/scripts/extract_edits.py (right): https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/ext... tools/clang/scripts/extract_edits.py:7: Clang tools are expected to emit edits like this: On 2016/12/27 07:30:14, dcheng wrote: > Not all clang tools, just rewriter tools, right? If a clang tool emits edits, then it is a rewriter tool + the edits should look like documented below. I tried to tweak the comment to make it clearer. https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/ext... tools/clang/scripts/extract_edits.py:50: unique_lines.add(line) On 2016/12/27 07:30:14, dcheng wrote: > On 2016/12/23 15:45:06, danakj_OOO_and_slow wrote: > > jw why bother with this, it will make this tool slower, and we already have > > de-duping in the apply script? > > The apply script reads all the edits before deduping. It could be a fairly > decent memory savings from dropping repeated edits in things like headers that > are included a lot. I haven't made a change here, but I am open to removing the deduping above: 1. For a big-scale refactoring, one would move the whole, unfiltered output from all platforms onto a linux box + deduped via sort -u. For small-scale, single-platform refactoring, maybe the deduping doesn't matter that much. 2. I like the simplification offered by not deduping: - Potential/not-yet-measured performance gains. - Possibly making the script reusable in other (hypothetical?) scenarios (if in the future the begin/end markers are configurable via a parameter?). https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/ext... tools/clang/scripts/extract_edits.py:51: print(line) On 2016/12/27 07:30:14, dcheng wrote: > For consistency, no () Done. https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/run... File tools/clang/scripts/run_tool.py (right): https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/run... tools/clang/scripts/run_tool.py:35: source files. These tools assumme the clang tool emits the edits in the On 2016/12/23 15:45:06, danakj_OOO_and_slow wrote: > assume Done. https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/run... tools/clang/scripts/run_tool.py:95: def _AtomicallyForwardToolOutput(stdout): On 2016/12/27 07:30:14, dcheng wrote: > On 2016/12/23 15:45:06, danakj_OOO_and_slow wrote: > > name this |text| or something not |stdout|, which would be used to refer to > the > > fd or something > > Rather than doing this, should we just 'return' the string and print to stdout > from the dispatcher? Done (printing to stdout from the dispatcher). Thanks for the suggestion - much cleaner this way. https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/run... tools/clang/scripts/run_tool.py:122: If status is True, then the generated edits are stored with the key "edits" On 2016/12/23 15:45:06, danakj_OOO_and_slow wrote: > needs update Done. Thanks for catching this. https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/run... tools/clang/scripts/run_tool.py:174: sys.stderr.flush() On 2016/12/23 15:45:06, danakj_OOO_and_slow wrote: > stderr doesnt need flush? Done. https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/run... tools/clang/scripts/run_tool.py:194: sys.stderr.flush() On 2016/12/23 15:45:06, danakj_OOO_and_slow wrote: > same Done. https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/tes... File tools/clang/scripts/test_tool.py (right): https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/tes... tools/clang/scripts/test_tool.py:51: # run_tools.py will skip them when applying replacements. On 2016/12/23 15:45:06, danakj_OOO_and_slow wrote: > run_tools.py? Done. https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/tes... tools/clang/scripts/test_tool.py:72: '/'] # compile.db created by test_tool.py uses absolute paths. On 2016/12/23 15:45:06, danakj_OOO_and_slow wrote: > isn't the path optional? why specify it then? The clang tool will *normally* emit paths relative to the build directory - for example: r:::../../third_party/WebKit/Source/wtf/TypeTraits.h:::2206:::18:::CheckAssignability Therefore (at least in the "normal" case) - we need the build directory to reconstruct full paths that the edits should be applied to. OTOH, in case of test_tool.py, the paths are absolute. Since fixing up relative paths is unconditional, we pass the root path here. Hmmm... maybe indeed the build directory path should be optional... Done. What I've ended up doing is check in _ParseEditsFromStdin whether path points to an existing file (which will be immediately true in case of test_tool.py) and only if not, then I try to resolve the path as relative to |build_directory|. I've also added code to emit errors if edits point to non-existant paths (with caching so that the error is only reported once per path).
LGTM with nits https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/ext... File tools/clang/scripts/extract_edits.py (right): https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/ext... tools/clang/scripts/extract_edits.py:50: unique_lines.add(line) On 2016/12/27 22:33:26, Łukasz Anforowicz wrote: > On 2016/12/27 07:30:14, dcheng wrote: > > On 2016/12/23 15:45:06, danakj_OOO_and_slow wrote: > > > jw why bother with this, it will make this tool slower, and we already have > > > de-duping in the apply script? > > > > The apply script reads all the edits before deduping. It could be a fairly > > decent memory savings from dropping repeated edits in things like headers that > > are included a lot. > > I haven't made a change here, but I am open to removing the deduping above: > > 1. For a big-scale refactoring, one would move the whole, unfiltered output from > all platforms onto a linux box + deduped via sort -u. For small-scale, > single-platform refactoring, maybe the deduping doesn't matter that much. > > 2. I like the simplification offered by not deduping: > - Potential/not-yet-measured performance gains. > - Possibly making the script reusable in other (hypothetical?) scenarios (if > in the future the begin/end markers are configurable via a parameter?). I don't feel strongly about this, but one easy way to figure out how many lines are deduped, right? https://codereview.chromium.org/2599193002/diff/40001/docs/clang_tool_refacto... File docs/clang_tool_refactoring.md (right): https://codereview.chromium.org/2599193002/diff/40001/docs/clang_tool_refacto... docs/clang_tool_refactoring.md:157: that contents of such header files are processed by the tool, the tool the tool => run_tool.py (there's too many different tools here, that the use of the generic 'tool' label is confusing) https://codereview.chromium.org/2599193002/diff/40001/docs/clang_tool_refacto... docs/clang_tool_refactoring.md:176: The tool will only apply edits to files actually under control of `git`. The tool => apply_edits.py https://codereview.chromium.org/2599193002/diff/40001/tools/clang/scripts/app... File tools/clang/scripts/apply_edits.py (right): https://codereview.chromium.org/2599193002/diff/40001/tools/clang/scripts/app... tools/clang/scripts/apply_edits.py:70: path_to_resolved_path = dict() Nit: = {} for consistency https://codereview.chromium.org/2599193002/diff/40001/tools/clang/scripts/app... tools/clang/scripts/apply_edits.py:81: sys.stderr.write('Edit applies to a non-existant file: %s\n' % path) Nit: existent https://codereview.chromium.org/2599193002/diff/40001/tools/clang/scripts/run... File tools/clang/scripts/run_tool.py (right): https://codereview.chromium.org/2599193002/diff/40001/tools/clang/scripts/run... tools/clang/scripts/run_tool.py:35: source files. These tools assume the clang tool emits the edits in the Nit: one space between sentences for consistency https://codereview.chromium.org/2599193002/diff/40001/tools/clang/scripts/run... tools/clang/scripts/run_tool.py:56: import threading Nit: remove https://codereview.chromium.org/2599193002/diff/40001/tools/clang/scripts/run... tools/clang/scripts/run_tool.py:111: "stdout_text" in the dictionary. Is this naming change a required fix for something?
Thanks for the reviews everyone. https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/ext... File tools/clang/scripts/extract_edits.py (right): https://codereview.chromium.org/2599193002/diff/20001/tools/clang/scripts/ext... tools/clang/scripts/extract_edits.py:50: unique_lines.add(line) On 2016/12/28 19:01:36, dcheng wrote: > On 2016/12/27 22:33:26, Łukasz Anforowicz wrote: > > On 2016/12/27 07:30:14, dcheng wrote: > > > On 2016/12/23 15:45:06, danakj_OOO_and_slow wrote: > > > > jw why bother with this, it will make this tool slower, and we already > have > > > > de-duping in the apply script? > > > > > > The apply script reads all the edits before deduping. It could be a fairly > > > decent memory savings from dropping repeated edits in things like headers > that > > > are included a lot. > > > > I haven't made a change here, but I am open to removing the deduping above: > > > > 1. For a big-scale refactoring, one would move the whole, unfiltered output > from > > all platforms onto a linux box + deduped via sort -u. For small-scale, > > single-platform refactoring, maybe the deduping doesn't matter that much. > > > > 2. I like the simplification offered by not deduping: > > - Potential/not-yet-measured performance gains. > > - Possibly making the script reusable in other (hypothetical?) scenarios > (if > > in the future the begin/end markers are configurable via a parameter?). > > I don't feel strongly about this, but one easy way to figure out how many lines > are deduped, right? I am sure that this is lots and lots of lines. I don't know if sort -u would be more efficient at removing the duplicates. All in all - I think it is fine to land as is and we can revisit later if needed. https://codereview.chromium.org/2599193002/diff/40001/docs/clang_tool_refacto... File docs/clang_tool_refactoring.md (right): https://codereview.chromium.org/2599193002/diff/40001/docs/clang_tool_refacto... docs/clang_tool_refactoring.md:157: that contents of such header files are processed by the tool, the tool On 2016/12/28 19:01:36, dcheng wrote: > the tool => run_tool.py (there's too many different tools here, that the use of > the generic 'tool' label is confusing) Done (but I think it is more correct to say "clang tool" here, not "run_tool.py"). https://codereview.chromium.org/2599193002/diff/40001/docs/clang_tool_refacto... docs/clang_tool_refactoring.md:176: The tool will only apply edits to files actually under control of `git`. On 2016/12/28 19:01:36, dcheng wrote: > The tool => apply_edits.py Done. https://codereview.chromium.org/2599193002/diff/40001/tools/clang/scripts/app... File tools/clang/scripts/apply_edits.py (right): https://codereview.chromium.org/2599193002/diff/40001/tools/clang/scripts/app... tools/clang/scripts/apply_edits.py:70: path_to_resolved_path = dict() On 2016/12/28 19:01:37, dcheng wrote: > Nit: = {} > for consistency Done. https://codereview.chromium.org/2599193002/diff/40001/tools/clang/scripts/app... tools/clang/scripts/apply_edits.py:81: sys.stderr.write('Edit applies to a non-existant file: %s\n' % path) On 2016/12/28 19:01:37, dcheng wrote: > Nit: existent Done. https://codereview.chromium.org/2599193002/diff/40001/tools/clang/scripts/run... File tools/clang/scripts/run_tool.py (right): https://codereview.chromium.org/2599193002/diff/40001/tools/clang/scripts/run... tools/clang/scripts/run_tool.py:35: source files. These tools assume the clang tool emits the edits in the On 2016/12/28 19:01:37, dcheng wrote: > Nit: one space between sentences for consistency Done. https://codereview.chromium.org/2599193002/diff/40001/tools/clang/scripts/run... tools/clang/scripts/run_tool.py:56: import threading On 2016/12/28 19:01:37, dcheng wrote: > Nit: remove Done. https://codereview.chromium.org/2599193002/diff/40001/tools/clang/scripts/run... tools/clang/scripts/run_tool.py:111: "stdout_text" in the dictionary. On 2016/12/28 19:01:37, dcheng wrote: > Is this naming change a required fix for something? danakj@ pointed out that |stdout| can be misunderstood as representing a fd or some other kind of file handle. This (and similar changes below) is addressing this part of the CR comments from her.
The CQ bit was checked by lukasza@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2599193002/#ps60001 (title: "Addressed remaining nits.")
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": 60001, "attempt_start_ts": 1482953595467170,
"parent_rev": "9b4165fc1e51e07a29c5ba88de6350e53d5c980a", "commit_rev":
"7780eda3fe6ace8490c089e071ae2260b671610b"}
Message was sent while issue was closed.
Description was changed from
==========
Split run_tool.py into run_tool.py, extract_edits.py and apply_edits.py
The split will allow generation of edits on multiple configs (e.g. linux
vs windows OR rel vs dbg) and merging the edits before applying them once:
$ tools/clang/scripts/run_tool.py rewrite_to_chrome_style \
--generate-compdb --all out/rel >run_tool.linux.rel.out
$ ...
$ cat run_tool.*.out \
| tools/clang/scripts/extract_edits.py \
| tools/clang/scripts/apply_edits.py
--generate-compdb --all out/rel >run_tool.linux.rel.out
Test steps:
- tools/clang/translation_unit/test_translation_unit.py
- tools/clang/scripts/test_tool.py rewrite_to_chrome_style
- manually running run_tool | extract_edits | apply_edits pipeline
on WTF and verifying that it still builds after the rename
BUG=598138
TEST=See "Test steps" above.
==========
to
==========
Split run_tool.py into run_tool.py, extract_edits.py and apply_edits.py
The split will allow generation of edits on multiple configs (e.g. linux
vs windows OR rel vs dbg) and merging the edits before applying them once:
$ tools/clang/scripts/run_tool.py rewrite_to_chrome_style \
--generate-compdb --all out/rel >run_tool.linux.rel.out
$ ...
$ cat run_tool.*.out \
| tools/clang/scripts/extract_edits.py \
| tools/clang/scripts/apply_edits.py
--generate-compdb --all out/rel >run_tool.linux.rel.out
Test steps:
- tools/clang/translation_unit/test_translation_unit.py
- tools/clang/scripts/test_tool.py rewrite_to_chrome_style
- manually running run_tool | extract_edits | apply_edits pipeline
on WTF and verifying that it still builds after the rename
BUG=598138
TEST=See "Test steps" above.
Review-Url: https://codereview.chromium.org/2599193002
==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from
==========
Split run_tool.py into run_tool.py, extract_edits.py and apply_edits.py
The split will allow generation of edits on multiple configs (e.g. linux
vs windows OR rel vs dbg) and merging the edits before applying them once:
$ tools/clang/scripts/run_tool.py rewrite_to_chrome_style \
--generate-compdb --all out/rel >run_tool.linux.rel.out
$ ...
$ cat run_tool.*.out \
| tools/clang/scripts/extract_edits.py \
| tools/clang/scripts/apply_edits.py
--generate-compdb --all out/rel >run_tool.linux.rel.out
Test steps:
- tools/clang/translation_unit/test_translation_unit.py
- tools/clang/scripts/test_tool.py rewrite_to_chrome_style
- manually running run_tool | extract_edits | apply_edits pipeline
on WTF and verifying that it still builds after the rename
BUG=598138
TEST=See "Test steps" above.
Review-Url: https://codereview.chromium.org/2599193002
==========
to
==========
Split run_tool.py into run_tool.py, extract_edits.py and apply_edits.py
The split will allow generation of edits on multiple configs (e.g. linux
vs windows OR rel vs dbg) and merging the edits before applying them once:
$ tools/clang/scripts/run_tool.py rewrite_to_chrome_style \
--generate-compdb --all out/rel >run_tool.linux.rel.out
$ ...
$ cat run_tool.*.out \
| tools/clang/scripts/extract_edits.py \
| tools/clang/scripts/apply_edits.py
--generate-compdb --all out/rel >run_tool.linux.rel.out
Test steps:
- tools/clang/translation_unit/test_translation_unit.py
- tools/clang/scripts/test_tool.py rewrite_to_chrome_style
- manually running run_tool | extract_edits | apply_edits pipeline
on WTF and verifying that it still builds after the rename
BUG=598138
TEST=See "Test steps" above.
Committed: https://crrev.com/f9b89e7bd49caf2c5137bd540528001d36b577bc
Cr-Commit-Position: refs/heads/master@{#440881}
==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/f9b89e7bd49caf2c5137bd540528001d36b577bc Cr-Commit-Position: refs/heads/master@{#440881} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
