New issue
Advanced search Search tips

Issue 672353 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 578344



Sign in to add a comment

rewrite_to_chrome_style: a range-based for loop gets mangled in platform/scheduler/base/work_queue_sets.cc

Project Member Reported by lukasza@chromium.org, Dec 8 2016

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) {
 
Oh, 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.
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.
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 *"
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)
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).
FWIW, I (as expected) didn't see any compile issues after starting to unconditionally avoid renaming of |begin|, |end|, etc.
Project Member

Comment 7 by bugdroid1@chromium.org, 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

Owner: lukasza@chromium.org
Status: Fixed (was: Untriaged)
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