rewrite_to_chrome_style: a range-based for loop gets mangled in platform/scheduler/base/work_queue_sets.cc |
||
Issue description
In third_party/WebKit/Source/platform/scheduler/base/work_queue_sets.cc the following rewriting happens:
- for (const OldestTaskEnqueueOrder& heap_value_pair : heap) {
+ for (const OldestTaskEnqueueOrder& heap_value_pair Beginnd heap) {
,
Dec 8 2016
The good news is that third_party/WebKit/Source/platform/scheduler/base/work_queue_sets.cc is the last source file of third_party/WebKit/Source/platform:platform that doesn't build after the rename (+ after fixing other known, but not necessarily landed yet issues). So - we are reeeeally close to being able to build this target after the rename.
,
Dec 8 2016
The trouble with #c1 comes from the condition on lines 248-249 below:
231 bool IsBlacklistedMethod(const clang::CXXMethodDecl& decl) {
...
245 // Iterator methods shouldn't be renamed to work with stl and range-for
246 // loops.
247 std::string ret_type = decl.getReturnType().getAsString();
248 if (ret_type.find("iterator") != std::string::npos ||
249 ret_type.find("Iterator") != std::string::npos) {
250 static const char* kIteratorBlacklist[] = {"begin", "end", "rbegin",
251 "rend"};
252 for (const auto& b : kIteratorBlacklist) {
253 if (name == b)
254 return true;
255 }
256 }
In case of the small repro case below:
37 namespace blink {
...
261 namespace blacklisting_of_renaming_of_begin_method {
262
263 template <typename T>
264 class IntrusiveHeap {
265 public:
266 // https://crbug.com/672353 : |begin| shouldn't be rewritten to |Begin|.
267 const T* begin() const { return nullptr; }
268 };
269
270 } // namespace blacklisting_of_renaming_of_begin_method
271
272 } // namespace blink
The |ret_type| from line 247 looks as follows:
ret_type = "const T *"
,
Dec 8 2016
https://codereview.chromium.org/1753563003 is where we tweaked the logic to look for iterator-ish types. I don't remember the exact cases that were breaking, but I think it's probably OK to have this generically skipped everywhere: it shouldn't affect too many things, and it's more important to have a non-broken build after running the tool. (We can manually fix up the incorrectly named begins later)
,
Dec 8 2016
FWIW, I see that return type checking comes from r378671 by danakj@. I am trying running the tool after making the blacklisting unconditional - I'll post an update if I see any fall out from this (I think this seems unlikely).
,
Dec 8 2016
FWIW, I (as expected) didn't see any compile issues after starting to unconditionally avoid renaming of |begin|, |end|, etc.
,
Dec 9 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d5e9f965676309e39c752db5e90e286b0e892260 commit d5e9f965676309e39c752db5e90e286b0e892260 Author: lukasza <lukasza@chromium.org> Date: Fri Dec 09 01:15:56 2016 Skipping renaming of begin/end/etc. should be unconditional. Before this CL, we tried to keep renaming |begin| -> |Begin| (and similarily for |end|, |rbegin|, |rend|) if the return type contained "iterator" in the typename. Unfortunately this meant that some undesired renamed were still happening (see the bug for more details). After this CL we will unconditionally avoid renaming |begin| (and |end|, |rbegin|, |rend|). We can do this rename where needed manually, after the "big rename" is done and sticks. BUG= 672353 Review-Url: https://codereview.chromium.org/2564663002 Cr-Commit-Position: refs/heads/master@{#437401} [modify] https://crrev.com/d5e9f965676309e39c752db5e90e286b0e892260/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp [modify] https://crrev.com/d5e9f965676309e39c752db5e90e286b0e892260/tools/clang/rewrite_to_chrome_style/tests/methods-expected.cc [modify] https://crrev.com/d5e9f965676309e39c752db5e90e286b0e892260/tools/clang/rewrite_to_chrome_style/tests/methods-original.cc [modify] https://crrev.com/d5e9f965676309e39c752db5e90e286b0e892260/tools/clang/rewrite_to_chrome_style/tests/template-expected.cc [modify] https://crrev.com/d5e9f965676309e39c752db5e90e286b0e892260/tools/clang/rewrite_to_chrome_style/tests/template-original.cc
,
Dec 9 2016
third_party/WebKit/Source/platform/scheduler/base/work_queue_sets.cc looks when running the renaming tool that includes the fix from #c7 |
||
►
Sign in to add a comment |
||
Comment 1 by lukasza@chromium.org
, Dec 8 2016Oh, and after attempting to fix this manually to for (const OldestTaskEnqueueOrder& heap_value_pair : heap) { it still doesn't compile, because it turns out that we've rewritten |begin| to |Begin| in third_party/WebKit/Source/platform/scheduler/base/intrusive_heap.h. This is rather suprising, because we supposedly blacklist rewriting of "begin" method in RewriteToChromeStyle.cpp via IsBlacklistedMethod.