New issue
Advanced search Search tips

Issue 711128 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 676588



Sign in to add a comment

Rebase tool gets confused by functions returning bools

Project Member Reported by dcheng@chromium.org, Apr 13 2017

Issue description

We have some logic to try to avoid incorrectly rewriting certain template parameters. However, bool is a macro on some platforms, and this confuses the logic to ignore unnamed template parameters.
 

Comment 1 by thakis@chromium.org, Apr 13 2017

>  bool is a macro on some platforms

!! On which platform?

Comment 2 by dcheng@chromium.org, Apr 13 2017

../../base/bind_unittest.cc:21:9: error: 'bool' macro redefined [-Werror,-Wmacro-redefined]        
#define bool MOO                                                                                    
        ^                                                                                           
/usr/local/google/home/dcheng/src/chrome/src/third_party/llvm-build/Release+Asserts/lib/clang/5.0.0$
include/stdbool.h:37:9: note: previous definition is here                                           
#define bool  bool

Apparently it's a GNU extension?
Project Member

Comment 3 by bugdroid1@chromium.org, Apr 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b029f9994f46b5f178b43740b3599832dcb763fa

commit b029f9994f46b5f178b43740b3599832dcb763fa
Author: Daniel Cheng <dcheng@chromium.org>
Date: Fri Apr 14 22:15:34 2017

rewrite_to_chrome_style: rewrite parameter names of functions that return bool

This function deletes code that was originally added to work around
https://llvm.org/bugs/show_bug.cgi?id=29145, since it incorrectly
rejected rewrites to function parameters if the function returned bool:
this is because bool is actually a macro on Linux (!!).

However, this exposes another bug for macro arguments that expanded in
multiple contexts: if the macro argument is expanded into the name of a
function and the name of a variable, the original text is incorrectly
rewritten twice: once to rewrite the name to be treated as a variable
name, and the second to uppercase the first letter.

While apply_edits.py should have caught this as a conflicting edit, it
does not for some reason--and moreover, in this case, it is preferable
to preserve function-style naming, as the effects are visible outside
the macro itself.

The fix for this is to create a global table of SourceLocations with
replacements, and avoid emitting subsequent replacements for the same
SourceLocation. This works because MatchFinder guarantees that match
ordering is equivalent to doing a pre-order traversal on the AST and
applying the matchers in the order in which they were added: in the AST,
the FunctionDecl always comes before the ParmVarDecl, allowing the
FunctionDecl naming to override the ParmVarDecl naming.

Bug:  711128 
Change-Id: I4333c782526ffd4abb62b0d8f881b11a24b0c7e7
Reviewed-on: https://chromium-review.googlesource.com/478471
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Lukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#464807}
[modify] https://crrev.com/b029f9994f46b5f178b43740b3599832dcb763fa/tools/blink_rename_merge_helper/COMPONENTS
[modify] https://crrev.com/b029f9994f46b5f178b43740b3599832dcb763fa/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
[add] https://crrev.com/b029f9994f46b5f178b43740b3599832dcb763fa/tools/clang/rewrite_to_chrome_style/tests/bool-is-macro-expected.cc
[add] https://crrev.com/b029f9994f46b5f178b43740b3599832dcb763fa/tools/clang/rewrite_to_chrome_style/tests/bool-is-macro-original.cc
[modify] https://crrev.com/b029f9994f46b5f178b43740b3599832dcb763fa/tools/clang/rewrite_to_chrome_style/tests/macros-expected.cc
[modify] https://crrev.com/b029f9994f46b5f178b43740b3599832dcb763fa/tools/clang/rewrite_to_chrome_style/tests/macros-original.cc
[modify] https://crrev.com/b029f9994f46b5f178b43740b3599832dcb763fa/tools/clang/rewrite_to_chrome_style/tests/variables-expected.cc

Comment 4 by dcheng@chromium.org, Apr 14 2017

Status: Fixed (was: Assigned)

Sign in to add a comment