Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2597863002:
rewrite_to_chrome_style: associate replacements with the affected file (Closed)
Descriptionrewrite_to_chrome_style: associate replacements with the affected file
To make the rebase helper more accurate, track edits on a per-file
basis. The theory is that within a file, a simple search-and-replace
approach can work more reliably without depending on as much contextual
information (e.g. detecting whether something is a method call or not).
BUG=676588
R=lukasza@chromium.org
Committed: https://chromium.googlesource.com/chromium/src/+/2bd7f8bf68e7f57558106183333d7763c8def486
Patch Set 1 #Patch Set 2 : actually implement stuff #
Total comments: 24
Patch Set 3 : . #Patch Set 4 : . #Patch Set 5 : . #
Total comments: 1
Messages
Total messages: 14 (4 generated)
Description was changed from ========== Moo BUG= ========== to ========== rewrite_to_chrome_style: associate replacements with the affected file To make the rebase helper more accurate, track edits on a per-file basis. The theory is that within a file, a simple search-and-replace approach can work more reliably without depending on as much contextual information (e.g. detecting whether something is a method call or not). BUG=676588 ==========
dcheng@chromium.org changed reviewers: + danakj@chromium.org, lukasza@chromium.org
+thakis, is it safe to assume that source filenames in Chromium are unique? https://codereview.chromium.org/2597863002/diff/20001/tools/clang/rewrite_to_... File tools/clang/rewrite_to_chrome_style/EditTracker.cpp (right): https://codereview.chromium.org/2597863002/diff/20001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/EditTracker.cpp:27: assert(!filename.empty() && "Can't track edit with no filename!"); For now, I'm going to be more aggressive and assert that this doesn't happen. I'm not entirely sure this logic is correct... but hopefully it is, it kind of makes sense that if you're still in a source location which is in scratch space, you're expanded from some other macro, so you need to go up the macro caller stack. https://codereview.chromium.org/2597863002/diff/20001/tools/clang/rewrite_to_... File tools/clang/rewrite_to_chrome_style/EditTracker.h (right): https://codereview.chromium.org/2597863002/diff/20001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/EditTracker.h:45: llvm::StringMap<EditInfo> tracked_edits_; Incidentally, LLVM solved the whole "std::map<std::string, ...>" is sad situation by just have a very specific implementation. Perhaps we should just do that in Chrome... https://codereview.chromium.org/2597863002/diff/20001/tools/clang/rewrite_to_... File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right): https://codereview.chromium.org/2597863002/diff/20001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:1283: // TODO(dcheng): There's a lot of match rewriters missing from this list. We'll add these in a followup. One tricky bit: it's not clear how we're going to disambiguate some of these (e.g. the matchers for using declarations). Perhaps the rebase helper should just look at the type of naming change to guess the original variable type? https://codereview.chromium.org/2597863002/diff/20001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:1290: llvm::outs() << "==== END TRACKED EDITS ====\n"; We're going to split run_tool.py in half: one half will run the tool and collect all the outputs. The other half of the tool will apply the edits from the scraping the collected output. Similarly, we'll build the rebase helper DB by scraping the output. https://codereview.chromium.org/2597863002/diff/20001/tools/clang/rewrite_to_... File tools/clang/rewrite_to_chrome_style/tests/methods-original.cc (right): https://codereview.chromium.org/2597863002/diff/20001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/tests/methods-original.cc:60: MyIterator rend() { return {}; } This was spamming my debug sessions, so I fixed it =)
Overall direction LGTM. The only major question I have is about desirability of jumping out of scratch space (and I also had some small suggestions / comments). https://codereview.chromium.org/2597863002/diff/20001/tools/clang/rewrite_to_... File tools/clang/rewrite_to_chrome_style/EditTracker.cpp (right): https://codereview.chromium.org/2597863002/diff/20001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/EditTracker.cpp:25: location = source_manager.getImmediateMacroCallerLoc(location); I don't understand why we want to do that here. We only care about rebase/merge-helping with renames of *full identifiers* - the rebase/merge-helper will not be able to deal with to##macroArg stuff, right? If a file contains rename of a *full identifier* than another replacement will be present in the EditTracker; otherwise (a replacement is in the scratch space) we can just ignore the replacement? https://codereview.chromium.org/2597863002/diff/20001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/EditTracker.cpp:27: assert(!filename.empty() && "Can't track edit with no filename!"); On 2016/12/22 10:26:22, dcheng wrote: > For now, I'm going to be more aggressive and assert that this doesn't happen. > > I'm not entirely sure this logic is correct... but hopefully it is, it kind of > makes sense that if you're still in a source location which is in scratch space, > you're expanded from some other macro, so you need to go up the macro caller > stack. Agreed, but please see me comment above. https://codereview.chromium.org/2597863002/diff/20001/tools/clang/rewrite_to_... File tools/clang/rewrite_to_chrome_style/EditTracker.h (right): https://codereview.chromium.org/2597863002/diff/20001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/EditTracker.h:11: #include "clang/Basic/SourceManager.h" very nitty nit / please ignore me: Can me forward declared? https://codereview.chromium.org/2597863002/diff/20001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/EditTracker.h:37: // <filename>:<prefix>:<original text>:<next text> typo:s/next/new/ nit/suggestion: s/<prefix>/<tag>/ (+ change the name of the parameter from |prefix| to |tag|?) For a moment I wanted to suggest line/delimiter format consistent with the format of edits, but then I realized that <original text> and <new test> should be *identifiers* and so should never include a ":" character. OTOH, if we're going to do something similar in the future for non-identifiers, then maybe it makes sense to start? But than YAGNI... I am not sure what do do here. https://codereview.chromium.org/2597863002/diff/20001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/EditTracker.h:42: EditTracker(const EditTracker&) = delete; Is this meant to be an equivalent of DISALLOW_COPY_*AND_ASSIGN*? I see that base/macros.h also deletes the assignment operator. https://codereview.chromium.org/2597863002/diff/20001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/EditTracker.h:45: llvm::StringMap<EditInfo> tracked_edits_; On 2016/12/22 10:26:22, dcheng wrote: > Incidentally, LLVM solved the whole "std::map<std::string, ...>" is sad > situation by just have a very specific implementation. > > Perhaps we should just do that in Chrome... I don't know what is meant by "sad situation" + I understand that this is not really related to this CL, but - isn't there a proposal for a future version of C++ stdlib that will allow map lookups by key to use alternative types? (i.e. allowing key lookup using |const char*| value, without requiring allocation of a separate/temporary |std::string|) If this is the case (and *this* is the sadness) then maybe we don't want to introduce a Chromium-specific type but rather wait until the standard gets less sad? https://codereview.chromium.org/2597863002/diff/20001/tools/clang/rewrite_to_... File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (left): https://codereview.chromium.org/2597863002/diff/20001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:1320: close(lockfd); I very much agree with the decision to just emit everything to stdout - this will play well with run_tool.py https://codereview.chromium.org/2597863002/diff/20001/tools/clang/rewrite_to_... File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right): https://codereview.chromium.org/2597863002/diff/20001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:691: edit_tracker_.Add(*result.SourceManager, loc, old_name, new_name); We might have to be careful when merging the change above with the changes for handling to##macroArg. The merge should be straightforward, but we might want to double check that the edit_tracker_.Add call: 1. Uses |old_name| and |new_name| (rather than the |expected_old_text| and |new_text|) 2. And I think using any |loc| ("original loc" VS "jumped out of scratch space loc") is okay here. Although maybe "original loc" is better... https://codereview.chromium.org/2597863002/diff/20001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:1283: // TODO(dcheng): There's a lot of match rewriters missing from this list. On 2016/12/22 10:26:22, dcheng wrote: > We'll add these in a followup. > > One tricky bit: it's not clear how we're going to disambiguate some of these > (e.g. the matchers for using declarations). Perhaps the rebase helper should > just look at the type of naming change to guess the original variable type? 1. I don't understand how the rebase helper will use the "prefix" / "tag" - will it behave differently for "fun" vs "var"? 2. Maybe just use "other" or "unknown" for the unresolved matchers? https://codereview.chromium.org/2597863002/diff/20001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:1290: llvm::outs() << "==== END TRACKED EDITS ====\n"; On 2016/12/22 10:26:22, dcheng wrote: > We're going to split run_tool.py in half: one half will run the tool and collect > all the outputs. The other half of the tool will apply the edits from the > scraping the collected output. > > Similarly, we'll build the rebase helper DB by scraping the output. Acknowledged. https://codereview.chromium.org/2597863002/diff/20001/tools/clang/rewrite_to_... File tools/clang/rewrite_to_chrome_style/tests/methods-original.cc (right): https://codereview.chromium.org/2597863002/diff/20001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/tests/methods-original.cc:60: MyIterator rend() { return {}; } On 2016/12/22 10:26:22, dcheng wrote: > This was spamming my debug sessions, so I fixed it =) Acknowledged.
On 2016/12/22 10:26:22, dcheng wrote: > +thakis, is it safe to assume that source filenames in Chromium are unique? You mean the qualified path (src/foo/a/b.cc)? I'd say yes, else there'd be two files at the same path. Just the basename? No. I believe I'm not understanding the question :-)
On 2016/12/22 17:45:16, Nico (swamped sorry) wrote: > On 2016/12/22 10:26:22, dcheng wrote: > > +thakis, is it safe to assume that source filenames in Chromium are unique? > > You mean the qualified path (src/foo/a/b.cc)? I'd say yes, else there'd be two > files at the same path. Just the basename? No. > > I believe I'm not understanding the question :-) I was hoping base names were unique-ish; I seem to recall some issue with .o's colliding in the past if this wasn't the case.
There are at least two layer.cc files for example. Not sure about in blink tho. On Thu, Dec 22, 2016 at 12:53 PM, <dcheng@chromium.org> wrote: > On 2016/12/22 17:45:16, Nico (swamped sorry) wrote: > > On 2016/12/22 10:26:22, dcheng wrote: > > > +thakis, is it safe to assume that source filenames in Chromium are > unique? > > > > You mean the qualified path (src/foo/a/b.cc)? I'd say yes, else there'd > be two > > files at the same path. Just the basename? No. > > > > I believe I'm not understanding the question :-) > > I was hoping base names were unique-ish; I seem to recall some issue with > .o's > colliding in the past if this wasn't the case. > > https://codereview.chromium.org/2597863002/ > -- 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.
https://codereview.chromium.org/2597863002/diff/20001/tools/clang/rewrite_to_... File tools/clang/rewrite_to_chrome_style/EditTracker.cpp (right): https://codereview.chromium.org/2597863002/diff/20001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/EditTracker.cpp:17: // bugs in the past when this wasn't true. I just dump the entire path now; it's easy enough to postprocess components out if we don't need them. https://codereview.chromium.org/2597863002/diff/20001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/EditTracker.cpp:25: location = source_manager.getImmediateMacroCallerLoc(location); On 2016/12/22 17:34:36, Łukasz Anforowicz wrote: > I don't understand why we want to do that here. We only care about > rebase/merge-helping with renames of *full identifiers* - the > rebase/merge-helper will not be able to deal with to##macroArg stuff, right? If > a file contains rename of a *full identifier* than another replacement will be > present in the EditTracker; otherwise (a replacement is in the scratch space) > we can just ignore the replacement? Without this logic to trace upwards, functions that are defined (without token pasting) inside macros won't be captured in the rebase table. It's true that we won't correctly handle the token pasting case (in theory we shouldn't even match against it), but I see that largely as a fringe case? https://codereview.chromium.org/2597863002/diff/20001/tools/clang/rewrite_to_... File tools/clang/rewrite_to_chrome_style/EditTracker.h (right): https://codereview.chromium.org/2597863002/diff/20001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/EditTracker.h:11: #include "clang/Basic/SourceManager.h" On 2016/12/22 17:34:36, Łukasz Anforowicz wrote: > very nitty nit / please ignore me: Can me forward declared? It could be. But I only bothered forward declaring the things LLVM style specifically asks not to include in headers (i.e. raw_ostream) (Current Google style encourages includes over forward decls, though Chromium itself doesn't follow this guideline) https://codereview.chromium.org/2597863002/diff/20001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/EditTracker.h:37: // <filename>:<prefix>:<original text>:<next text> On 2016/12/22 17:34:36, Łukasz Anforowicz wrote: > typo:s/next/new/ > > nit/suggestion: s/<prefix>/<tag>/ > (+ change the name of the parameter from |prefix| to |tag|?) > > For a moment I wanted to suggest line/delimiter format consistent with the > format of edits, but then I realized that <original text> and <new test> should > be *identifiers* and so should never include a ":" character. OTOH, if we're > going to do something similar in the future for non-identifiers, then maybe it > makes sense to start? But than YAGNI... I am not sure what do do here. We already have a hack for newlines: https://cs.chromium.org/chromium/src/tools/clang/scripts/run_tool.py?rcl=0&l=108 If it comes down to it, we can extend it for : as well. Done with the rename suggestions. https://codereview.chromium.org/2597863002/diff/20001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/EditTracker.h:42: EditTracker(const EditTracker&) = delete; On 2016/12/22 17:34:36, Łukasz Anforowicz wrote: > Is this meant to be an equivalent of DISALLOW_COPY_*AND_ASSIGN*? I see that > base/macros.h also deletes the assignment operator. Done. https://codereview.chromium.org/2597863002/diff/20001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/EditTracker.h:45: llvm::StringMap<EditInfo> tracked_edits_; On 2016/12/22 17:34:36, Łukasz Anforowicz wrote: > On 2016/12/22 10:26:22, dcheng wrote: > > Incidentally, LLVM solved the whole "std::map<std::string, ...>" is sad > > situation by just have a very specific implementation. > > > > Perhaps we should just do that in Chrome... > > I don't know what is meant by "sad situation" + I understand that this is not > really related to this CL, but - isn't there a proposal for a future version of > C++ stdlib that will allow map lookups by key to use alternative types? (i.e. > allowing key lookup using |const char*| value, without requiring allocation of a > separate/temporary |std::string|) If this is the case (and *this* is the > sadness) then maybe we don't want to introduce a Chromium-specific type but > rather wait until the standard gets less sad? It doesn't help with unordered sets, and the transparent comparator is a bit awkward to use. https://codereview.chromium.org/2597863002/diff/20001/tools/clang/rewrite_to_... File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right): https://codereview.chromium.org/2597863002/diff/20001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:691: edit_tracker_.Add(*result.SourceManager, loc, old_name, new_name); On 2016/12/22 17:34:36, Łukasz Anforowicz wrote: > We might have to be careful when merging the change above with the changes for > handling to##macroArg. The merge should be straightforward, but we might want > to double check that the edit_tracker_.Add call: > 1. Uses |old_name| and |new_name| (rather than the |expected_old_text| and > |new_text|) > 2. And I think using any |loc| ("original loc" VS "jumped out of scratch space > loc") is okay here. Although maybe "original loc" is better... Yeah I'm not sure what the best approach is. I think we want the original loc, in case we do any processing though? https://codereview.chromium.org/2597863002/diff/20001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:1283: // TODO(dcheng): There's a lot of match rewriters missing from this list. On 2016/12/22 17:34:36, Łukasz Anforowicz wrote: > On 2016/12/22 10:26:22, dcheng wrote: > > We'll add these in a followup. > > > > One tricky bit: it's not clear how we're going to disambiguate some of these > > (e.g. the matchers for using declarations). Perhaps the rebase helper should > > just look at the type of naming change to guess the original variable type? > > 1. I don't understand how the rebase helper will use the "prefix" / "tag" - will > it behave differently for "fun" vs "var"? > > 2. Maybe just use "other" or "unknown" for the unresolved matchers? I think I'll tweak this to infer based on the type of rename. It's useful because you can have int mainFunction() { int mainFunction = 0; } And we'll rename those differently.
Message was sent while issue was closed.
Description was changed from ========== rewrite_to_chrome_style: associate replacements with the affected file To make the rebase helper more accurate, track edits on a per-file basis. The theory is that within a file, a simple search-and-replace approach can work more reliably without depending on as much contextual information (e.g. detecting whether something is a method call or not). BUG=676588 ========== to ========== rewrite_to_chrome_style: associate replacements with the affected file To make the rebase helper more accurate, track edits on a per-file basis. The theory is that within a file, a simple search-and-replace approach can work more reliably without depending on as much contextual information (e.g. detecting whether something is a method call or not). BUG=676588 R=lukasza@chromium.org Committed: https://crrev.com/2bd7f8bf68e7f57558106183333d7763c8def486 Cr-Commit-Position: refs/heads/master@{#440496} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/2bd7f8bf68e7f57558106183333d7763c8def486 Cr-Commit-Position: refs/heads/master@{#440496}
Message was sent while issue was closed.
Description was changed from ========== rewrite_to_chrome_style: associate replacements with the affected file To make the rebase helper more accurate, track edits on a per-file basis. The theory is that within a file, a simple search-and-replace approach can work more reliably without depending on as much contextual information (e.g. detecting whether something is a method call or not). BUG=676588 R=lukasza@chromium.org Committed: https://crrev.com/2bd7f8bf68e7f57558106183333d7763c8def486 Cr-Commit-Position: refs/heads/master@{#440496} ========== to ========== rewrite_to_chrome_style: associate replacements with the affected file To make the rebase helper more accurate, track edits on a per-file basis. The theory is that within a file, a simple search-and-replace approach can work more reliably without depending on as much contextual information (e.g. detecting whether something is a method call or not). BUG=676588 R=lukasza@chromium.org Committed: https://chromium.googlesource.com/chromium/src/+/2bd7f8bf68e7f57558106183333d... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as 2bd7f8bf68e7f57558106183333d7763c8def486 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/2597863002/diff/80001/tools/clang/rewrite_to_... File tools/clang/rewrite_to_chrome_style/EditTracker.h (right): https://codereview.chromium.org/2597863002/diff/80001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/EditTracker.h:25: // Simple class that tracks the edits made by path. Used to dump the databaes the databaes are bae
Message was sent while issue was closed.
On 2016/12/22 18:14:08, danakj_OOO_and_slow wrote: > There are at least two layer.cc files for example. Not sure about in blink > tho. > > On Thu, Dec 22, 2016 at 12:53 PM, <mailto:dcheng@chromium.org> wrote: > > > On 2016/12/22 17:45:16, Nico (swamped sorry) wrote: > > > On 2016/12/22 10:26:22, dcheng wrote: > > > > +thakis, is it safe to assume that source filenames in Chromium are > > unique? > > > > > > You mean the qualified path (src/foo/a/b.cc)? I'd say yes, else there'd > > be two > > > files at the same path. Just the basename? No. > > > > > > I believe I'm not understanding the question :-) > > > > I was hoping base names were unique-ish; There are definitely basename collision (e.g, Trace_event) > I seem to recall some issue with > > .o's > > colliding in the past if this wasn't the case. I recall being bitten by something similar during the gyp - > transition but I think that is limited to the scope of a gn target. > > > > https://codereview.chromium.org/2597863002/ > > > > -- > 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 mailto:chromium-reviews+unsubscribe@chromium.org. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
