[PATCH] D22465: [clang-rename] introduce better symbol finding and add few more tests

[Kirill Bobyrev via cfe-commits]

omtcyfz added inline comments.

================
Comment at: clang-rename/USRFinder.cpp:94
@@ -91,4 +93,3 @@
       const auto *Decl = NameLoc.getNestedNameSpecifier()->getAsNamespace();
-      if (Decl && !setResult(Decl, NameLoc.getLocalBeginLoc(),
-                             Decl->getNameAsString().length()))
-        return false;
+      if (Decl) {
+        setResult(Decl, NameLoc.getLocalBeginLoc(), NameLoc.getLocalEndLoc());
----------------
alexfh wrote:
> nit: No need for braces around single-line `if` bodies.
Well, I'm really happy with braces everywhere. The codestyle doesn't prohibit it and I think it's more readable than no braces around single statements in logical blocks.

================
Comment at: clang-rename/USRFinder.h:49
@@ +48,3 @@
+// FIXME: This wouldn't be needed if
+// RecursiveASTVisitor<T>::VisitNestedNameSpecifier would have been implemented.
+class NestedNameSpecifierLocFinder : public MatchFinder::MatchCallback {
----------------
alexfh wrote:
> We should implement it then ;)
Yes, it's in my TODO list. But I suppose it's better to fix clang-rename first and get back to it a bit later.

As we discussed, there will probably be no need in RecursiveASTVisitors at all in the end.

Anyway, the patch is already in the state when nobody wants to review it :D I'd be happy just to land it and to continue with all the other bugs present in the tool.

================
Comment at: test/clang-rename/ComplicatedClassType.cpp:1
@@ -1,2 +1,2 @@
-// Unsupported test.
 // RUN: cat %s > %t.cpp
+// RUN: clang-rename -offset=220 -new-name=Bar %t.cpp -i --
----------------
alexfh wrote:
> I guess, this test will need `// REQUIRES: shell`. You can try to commit without it, but watch windows buildbots closely.
This test? There's `RUN: cat %s > %t.cpp` in every single one of them, I'm not sure what's the issue.


https://reviews.llvm.org/D22465