Tweak |in_blink_namespace|, to look at ancestors *and also* at qualifiers.

Tweak |in_blink_namespace|, to look at ancestors *and also* at qualifiers.

BUG=640688
Committed: https://crrev.com/f5e43f031c552c4461cc0451c54662c9f099dbe0
Cr-Commit-Position: refs/heads/master@{#415101}

[Łukasz Anforowicz, 2016-08-24 19:20:03]

Description was changed from

==========
Tweak |in_blink_namespace|, to use hasAncestor *and also* hasQualifier(...).

BUG=640688
==========

to

==========
Tweak |in_blink_namespace|, to look at ancestors *and also* at qualifiers.

BUG=640688
==========

[Łukasz Anforowicz, 2016-08-24 19:20:20]

lukasza@chromium.org changed reviewers:
+ dcheng@chromium.org

[Łukasz Anforowicz, 2016-08-24 19:20:21]

Daniel, can you take a look please?

https://codereview.chromium.org/2271203004/diff/1/tools/clang/rewrite_to_chro...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2271203004/diff/1/tools/clang/rewrite_to_chro...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:193:
declaratorDeclHasQualifier,
Ideally I would name this |hasQualifier|, but if I do this, I always get "call
to 'hasQualifier' is ambiguous" error (conflict comes from the builtin
hasQualifier) and I cannot work around this error with any number of "decl(...)"
or "declaratorDecl(...)" or "nestedNameSpecifier(...)"... :-/

[dcheng, 2016-08-25 00:33:13]

https://codereview.chromium.org/2271203004/diff/20001/tools/clang/rewrite_to_...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2271203004/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:195: InnerMatcher)
{
Is there any chance we can use the nestedNameSpecifier() matcher? There's some
examples in the clang tests that suggest this might work:
https://cs.chromium.org/chromium/src/third_party/llvm/tools/clang/unittests/A...

https://codereview.chromium.org/2271203004/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:843:
hasPrefix(specifiesNamespace(blink_namespace_decl)))));
How come we need both? It would be good to expand the comment to make the reason
clear.

[Łukasz Anforowicz, 2016-08-25 21:08:09]

Daniel - thanks for the feedback and improvement suggestions.  Can you take
another look please?

https://codereview.chromium.org/2271203004/diff/20001/tools/clang/rewrite_to_...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2271203004/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:195: InnerMatcher)
{
On 2016/08/25 00:33:12, dcheng wrote:
> Is there any chance we can use the nestedNameSpecifier() matcher? There's some
> examples in the clang tests that suggest this might work:
>
https://cs.chromium.org/chromium/src/third_party/llvm/tools/clang/unittests/A...

Interesting.

FWIW, the AST for ...

    template <typename Allocator = PartitionAllocator>
    void test() {
      // https://crbug.com/582315: |Allocator::method| is a
      // DependentScopeDeclRefExpr.
      if (std::is_class<Allocator>::value)  // Shouldn't rename |value|.
        Allocator::method();                // Should rename |method|.
    }

... looks like this:

    |-FunctionTemplateDecl 0x5867738 <line:215:1, line:221:1> line:216:6 test
    | |-TemplateTypeParmDecl 0x58675f0 <line:215:11, col:32> col:20 referenced
typename Allocator
    | | `-TemplateArgument type 'class
blink::test_new_array_bug::PartitionAllocator'
    | `-FunctionDecl 0x5867690 <line:216:1, line:221:1> line:216:6 test 'void
(void)'
    |   `-CompoundStmt 0x5867a20 <col:13, line:221:1>
    |     `-IfStmt 0x58679e8 <line:219:3, line:220:23>
    |       |-<<<NULL>>>
    |       |-<<<NULL>>>
    |       |-DependentScopeDeclRefExpr 0x58678d8 <line:219:7, col:33>
'<dependent type>' lvalue
    |       |-CallExpr 0x58679c0 <line:220:5, col:23> '<dependent type>'
    |       | `-DependentScopeDeclRefExpr 0x5867980 <col:5, col:16> '<dependent
type>' lvalue
    |       `-<<<NULL>>>

Note that, there is no NestedNameSpecifier node above.  But... the following
*does* work:

-     declaratorDecl(declaratorDeclHasQualifier(...
+     declaratorDecl(has(nestedNameSpecifier(...

Done.  Thanks for the suggestion.

PS. I can probably also apply this suggestion to the other places where I
introduce hasQualifier-like matchers - in
https://codereview.chromium.org/2256913002.  I'll give it a try in a minute.

https://codereview.chromium.org/2271203004/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:843:
hasPrefix(specifiesNamespace(blink_namespace_decl)))));
On 2016/08/25 00:33:12, dcheng wrote:
> How come we need both? It would be good to expand the comment to make the
reason
> clear.

I've added a test in methods-original.cc + verified that
1. with only specifiesNamespace(...), WTF::Class::method doesn't get rewritten
(in the newly added test)
2. with only hasPrefix(specifiesNamespace(...)), WTF::function doesn't get
rewritten (in the test present in the previous patchset)

I've expanded the comment above to clarify which parts of the matcher are
responsible for which parts of the matched code.

WDYT?

PS. I wondered whether to split the matcher, but I am not sure if more verbose
code would be cleaner.  Please shout if you think that matching (1) and (2) in
two pieces of the comments is not readable (the alternative being splitting the
matcher into 2 halves, right? :-/ ).

[Łukasz Anforowicz, 2016-08-25 23:35:52]

Also - heads-up that I've realized that (in patchset #5) I need to add
or_hasAncestor(decl_has_qualifier_to_blink_namespace) to the matcher - otherwise
the parameters and variables would not get rewritten below (from
functions-original.cc):

void blink::bug640688(int myParam) {
  char myVariable = 'c';
}

[dcheng, 2016-08-29 21:12:17]

https://codereview.chromium.org/2271203004/diff/80001/tools/clang/rewrite_to_...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2271203004/diff/80001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:601: //   void
WTF::Class::method() {}  // (2)
As mentioned offline, this doesn't match something like
WTF::internal::Class::method. We could write a custom matcher, which would let
us simplify lines 605-606.

I leave it up to you if you want to do this or not: it's a "nice to have" but
almost certainly overkill.

(If we don't do it, perhaps update this comment to note that it only handles one
level of nesting though)

[dcheng, 2016-08-29 21:13:14]

PS6 LGTM

[Łukasz Anforowicz, 2016-08-29 21:18:44]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2016-08-29 21:47:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 04:36:10]

Description was changed from

==========
Tweak |in_blink_namespace|, to look at ancestors *and also* at qualifiers.

BUG=640688
==========

to

==========
Tweak |in_blink_namespace|, to look at ancestors *and also* at qualifiers.

BUG=640688
==========

[commit-bot: I haz the power, 2016-08-30 04:36:11]

Committed patchset #6 (id:90001)

[commit-bot: I haz the power, 2016-08-30 04:39:13]

Description was changed from

==========
Tweak |in_blink_namespace|, to look at ancestors *and also* at qualifiers.

BUG=640688
==========

to

==========
Tweak |in_blink_namespace|, to look at ancestors *and also* at qualifiers.

BUG=640688

Committed: https://crrev.com/f5e43f031c552c4461cc0451c54662c9f099dbe0
Cr-Commit-Position: refs/heads/master@{#415101}
==========

[commit-bot: I haz the power, 2016-08-30 04:39:14]

Patchset 6 (id:??) landed as
https://crrev.com/f5e43f031c552c4461cc0451c54662c9f099dbe0
Cr-Commit-Position: refs/heads/master@{#415101}