Handling of UnresolvedUsingValueDecl nodes.

Handling of UnresolvedUsingValueDecl nodes.

BUG=640016
Committed: https://crrev.com/ff1d946e89c5fc53fb7c6f2e41b1bcbd9a06b238
Cr-Commit-Position: refs/heads/master@{#435259}

[Łukasz Anforowicz, 2016-08-23 20:46:28]

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

[Łukasz Anforowicz, 2016-08-23 20:46:28]

Daniel, can you take a look please?

I think this CL is ready for a review - after applying all the 3 CLs
below, and running the tool on Source/wtf, things start to compile
without any errors (so far I've tested building of PrintStream.o).
This increases my confidence that the 3 CLs below really are moving
things in the right direction:
- https://codereview.chromium.org/2246263002
- https://codereview.chromium.org/2274653002
- https://codereview.chromium.org/2256913002

[dcheng, 2016-09-10 04:00:56]

https://codereview.chromium.org/2274653002/diff/200001/tools/clang/rewrite_to...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (left):

https://codereview.chromium.org/2274653002/diff/200001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:467: static const
char* GetType() { return "NamedDecl"; }
While unused, these were helpful for debugging in the past. Let's leave them?

https://codereview.chromium.org/2274653002/diff/200001/tools/clang/rewrite_to...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2274653002/diff/200001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:178: bool
IsBlacklistedMethod(const std::string& name,
Pass as llvm::StringRef here, so we don't need to coerce to string.

https://codereview.chromium.org/2274653002/diff/200001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:180: if (decl &&
decl->isStatic())
It feels like we should just have two versions of this: one for CXXMethodDecl
and one for UnresolvedUsingValueDecl. We can pull out some of the common checks,
but some of the checks here will simply always be skipped for
UnresolvedUsingValueDecl.

https://codereview.chromium.org/2274653002/diff/200001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:200:
ret_type.find("Iterator") != std::string::npos;
For this, we can just have a simple shared method to determine if the thing is
in an iterator, with the return type checks only in the CXXMethodDecl version.

https://codereview.chromium.org/2274653002/diff/200001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:597: };
Nit: let's move the DeclRewriterBase using decls up here to group things and
give them a logical divider.

https://codereview.chromium.org/2274653002/diff/200001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:610: bool
IsCallee(const clang::Expr& expr, clang::ASTContext& context) {
Can this logic be encapsulated into a matcher?

https://codereview.chromium.org/2274653002/diff/200001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:622: // Heuristic -
assuming that |using Base::foo| refers to a method.
We can guess from the name though, right?

https://codereview.chromium.org/2274653002/diff/200001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:653: const
std::string& old_name,
Pass this as a llvm::StringRef.

https://codereview.chromium.org/2274653002/diff/200001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:656: if
(old_name.substr(0, strlen(kBlinkFieldPrefix)) == kBlinkFieldPrefix) {
Then this can just use llvm::StringRef::startswith

[Łukasz Anforowicz, 2016-09-12 19:52:56]

Thanks for the review - can you take another look please?

https://codereview.chromium.org/2274653002/diff/200001/tools/clang/rewrite_to...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (left):

https://codereview.chromium.org/2274653002/diff/200001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:467: static const
char* GetType() { return "NamedDecl"; }
On 2016/09/10 04:00:55, dcheng wrote:
> While unused, these were helpful for debugging in the past. Let's leave them?

Done.

https://codereview.chromium.org/2274653002/diff/200001/tools/clang/rewrite_to...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2274653002/diff/200001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:178: bool
IsBlacklistedMethod(const std::string& name,
On 2016/09/10 04:00:55, dcheng wrote:
> Pass as llvm::StringRef here, so we don't need to coerce to string.

Done.

This is how I started actually, but then got confused because it turns out that
llvm::StringRef is not a drop-in replacement for std::string (i.e. some
llvm::StringRef methods mutate their argument when the equivalent std::string
methods wouldn't;  I think this might be substr, but I forgot).  OTOH, I see
that it works fine now.

https://codereview.chromium.org/2274653002/diff/200001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:180: if (decl &&
decl->isStatic())
On 2016/09/10 04:00:55, dcheng wrote:
> It feels like we should just have two versions of this: one for CXXMethodDecl
> and one for UnresolvedUsingValueDecl. We can pull out some of the common
checks,
> but some of the checks here will simply always be skipped for
> UnresolvedUsingValueDecl.

In the latest patchset I have 1) reverted changes to IsBlacklistedMethod (so
that it again always works against CXXMethodDecl) and 2) introduced a separate
IsBlacklistedFunctionOrMethodName.  I've thought a little bit on how to avoid
duplication, but everything I thought about seems too heavy so I didn't
deduplicate and I think this is okay.  The best deduplication I could think of
was to introduce GetSetOfBlacklistedIteratorNames,
GetSetOfBlacklistedFunctionNames, GetSetOfUnconditionallyBlacklistedMethodNames,
etc. (with the set(s) being constructed lazily in the functions + reused from
both IsBlacklistedMethod and from IsBlacklistedFunctionOrMethodName).

https://codereview.chromium.org/2274653002/diff/200001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:200:
ret_type.find("Iterator") != std::string::npos;
On 2016/09/10 04:00:55, dcheng wrote:
> For this, we can just have a simple shared method to determine if the thing is
> in an iterator, with the return type checks only in the CXXMethodDecl version.

I think this comment is not applicable to the most recent patchset.

https://codereview.chromium.org/2274653002/diff/200001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:597: };
On 2016/09/10 04:00:55, dcheng wrote:
> Nit: let's move the DeclRewriterBase using decls up here to group things and
> give them a logical divider.

Done.

https://codereview.chromium.org/2274653002/diff/200001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:610: bool
IsCallee(const clang::Expr& expr, clang::ASTContext& context) {
On 2016/09/10 04:00:55, dcheng wrote:
> Can this logic be encapsulated into a matcher?

Done.  Thanks for helping figure out that I need |callExpr| in there :-)

https://codereview.chromium.org/2274653002/diff/200001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:622: // Heuristic -
assuming that |using Base::foo| refers to a method.
On 2016/09/10 04:00:55, dcheng wrote:
> We can guess from the name though, right?

Yes, we can guess from the name - we do it in
GuessNameForUnresolvedDependentNode and we use IsCallee as a secondary
verification that we are really dealing with a function/method.

In theory I could duplicate name checks from GuessNameForUnresolvedDependentNode
into IsCallee, but I am not sure if this would be desirable.

FWIW, I've added a comment saying that name checks will be done elsewhere.

https://codereview.chromium.org/2274653002/diff/200001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:653: const
std::string& old_name,
On 2016/09/10 04:00:56, dcheng wrote:
> Pass this as a llvm::StringRef.

Done.

https://codereview.chromium.org/2274653002/diff/200001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:656: if
(old_name.substr(0, strlen(kBlinkFieldPrefix)) == kBlinkFieldPrefix) {
On 2016/09/10 04:00:55, dcheng wrote:
> Then this can just use llvm::StringRef::startswith

Done.

https://codereview.chromium.org/2274653002/diff/200001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:657: std::string
field_name = old_name.substr(strlen(kBlinkFieldPrefix));
This is the reason why in the end I've used std::string instead of
llvm::StringRef.  If |old_name| is a llvm::StringRef, then I get results which I
don't understand:

--- tools/clang/rewrite_to_chrome_style/tests/template-expected.cc
+++ tools/clang/rewrite_to_chrome_style/tests/template-actual.cc
@@ -132,8 +132,8 @@
  private:
   using Base = BaseClass<T>;
   // https://crbug.com/640016: Need to rewrite |m_size| into |size_|.
-  using Base::size_;
-  void Method() { size_ = 123; }
+  using Base::zeze_;
+  void Method() { zeze_ = 123; }
 };
 
 }  // namespace test_inherited_field

if |old_name| is llvm::StringRef and the code says:

      std::string field_name = old_name.substr(strlen(kBlinkFieldPrefix));

instead of

      std::string field_name = old_name.str().substr(strlen(kBlinkFieldPrefix));


Things work in the latest patchset, but the |.str()| is a bit of an eye-sore to
me... :-/

[Łukasz Anforowicz, 2016-11-22 17:41:10]

Daniel, do you have any more feedback?  Or does patchset 12 look okay to land? 
We should probably try to wrap-up the known issues and attempt to flush out any
remaining unknown issues in December (hoping to be ready for the big CL during
the quiet period in the last week of December).

[dcheng, 2016-11-28 22:05:48]

https://codereview.chromium.org/2274653002/diff/220001/tools/clang/rewrite_to...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2274653002/diff/220001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:688: auto matcher =
stmt(hasParent(callExpr(callee(equalsNode(&expr)))));
Why is it important to test this for UnresolvedMemberExpr? Or more simply, what
breaks if this simply always returns true like the UnresolvedUsingValueDecl
version?

https://codereview.chromium.org/2274653002/diff/220001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:1060: //    using
Base::m_size; <- matched by |unresolved_using_value_decl_matcher|.
Super minor nit: Add a // here, or consider just moving the comments out-of-line
to simplify the wrapping + alignment.

[Łukasz Anforowicz, 2016-11-29 00:45:42]

https://codereview.chromium.org/2274653002/diff/220001/tools/clang/rewrite_to...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2274653002/diff/220001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:688: auto matcher =
stmt(hasParent(callExpr(callee(equalsNode(&expr)))));
On 2016/11/28 22:05:48, dcheng wrote:
> Why is it important to test this for UnresolvedMemberExpr? Or more simply,
what
> breaks if this simply always returns true like the UnresolvedUsingValueDecl
> version?

I don't remember :-(.  I should have added tests when working on this.  I only
vaguely remember that some things were renamed that shouldn't be renamed, but I
don't remember what they were exactly.

I guess what I can do is remove IsCallee entirely from this CL (since the tests
I do have pass without it) and experiment with it to the next CL.  If something
fails without IsCallee, we should find out soon (after the next CL all of WTF
should hopefully build after running the tool).

https://codereview.chromium.org/2274653002/diff/220001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:1060: //    using
Base::m_size; <- matched by |unresolved_using_value_decl_matcher|.
On 2016/11/28 22:05:48, dcheng wrote:
> Super minor nit: Add a // here, or consider just moving the comments
out-of-line
> to simplify the wrapping + alignment.

Done.  (this also unifies the formatting of the comment wrt to the one two lines
below).

[Łukasz Anforowicz, 2016-11-29 20:01:05]

https://codereview.chromium.org/2274653002/diff/220001/tools/clang/rewrite_to...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2274653002/diff/220001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:688: auto matcher =
stmt(hasParent(callExpr(callee(equalsNode(&expr)))));
On 2016/11/29 00:45:42, Łukasz Anforowicz wrote:
> On 2016/11/28 22:05:48, dcheng wrote:
> > Why is it important to test this for UnresolvedMemberExpr? Or more simply,
> what
> > breaks if this simply always returns true like the UnresolvedUsingValueDecl
> > version?
> 
> I don't remember :-(.  I should have added tests when working on this.  I only
> vaguely remember that some things were renamed that shouldn't be renamed, but
I
> don't remember what they were exactly.
> 
> I guess what I can do is remove IsCallee entirely from this CL (since the
tests
> I do have pass without it) and experiment with it to the next CL.  If
something
> fails without IsCallee, we should find out soon (after the next CL all of WTF
> should hopefully build after running the tool).

Found it!  I've added a regression test in the follow-up CL here:
https://codereview.chromium.org/2256913002/patch/220001/230003

[dcheng, 2016-11-29 22:40:44]

lgtm

[Łukasz Anforowicz, 2016-11-29 22:48:46]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2016-11-29 22:49:26]

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-11-30 00:51:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 00:51:35]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[Łukasz Anforowicz, 2016-11-30 00:58:31]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2016-11-30 00:59:36]

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-11-30 03:03:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 03:03:31]

Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[Łukasz Anforowicz, 2016-11-30 14:49:33]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2016-11-30 14:49:48]

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-11-30 14:52:54]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1480517373536140,
"parent_rev": "9ff68771b039df05b78b08809bc7c4e819823930", "commit_rev":
"8e4a7898d311b21a5374a80e20c67f05aeccdafe"}

[commit-bot: I haz the power, 2016-11-30 14:53:20]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-11-30 14:55:10]

Description was changed from

==========
Handling of UnresolvedUsingValueDecl nodes.

BUG=640016
==========

to

==========
Handling of UnresolvedUsingValueDecl nodes.

BUG=640016

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

[commit-bot: I haz the power, 2016-11-30 14:55:11]

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