[PATCH] D13931: [Tooling] Add a utility function to replace one nested name with another.

[Manuel Klimek via cfe-commits]

klimek added inline comments.

================
Comment at: include/clang/Tooling/Core/Lookup.h:37-38
@@ +36,4 @@
+/// \param FromDecl The declaration to which the nested name points.
+/// \param ReplacementString The replacement nested name. Should be fully
+///                          qualified.
+/// \returns The new name to be inserted in place of the current nested name.
----------------
Don't most tests violate the "should be fully qualified" requirement here?

================
Comment at: lib/Tooling/Core/Lookup.cpp:60
@@ +59,3 @@
+
+    // Otherwise strip of redundant namespace qualifications from the new name.
+    std::string NS =
----------------
s/of/off/

================
Comment at: lib/Tooling/Core/Lookup.cpp:63-66
@@ +62,6 @@
+        cast<NamespaceDecl>(DeclA)->getQualifiedNameAsString() + "::";
+    if (NewName.startswith("::"))
+      NS = "::" + NS;
+    if (NewName.startswith(NS))
+      return NewName.substr(NS.size());
+    DeclA = DeclA->getParent();
----------------
If NewName is fully qualified, why would we ever want to allow that to match from a non-global namespace?

namespace a { namespace b { namespace c {
void foo();
}}}

-> if NewName is "::c::bar" (for whatever reason), this should probably not silently go to bar inside 'c', or to c::bar inside 'b'.

================
Comment at: lib/Tooling/Core/Lookup.cpp:72
@@ +71,3 @@
+/// Check if the name specifier begins with a written "::".
+static bool isNameSpecifierGlobal(const NestedNameSpecifier *NNS) {
+  while (NNS) {
----------------
Isn't that more like isFullyQualified then?

================
Comment at: unittests/Tooling/LookupTest.cpp:19-20
@@ +18,4 @@
+  bool VisitCallExpr(CallExpr *Expr) {
+    if (OnCall)
+      OnCall(Expr);
+    return true;
----------------
Is this ever not true?


http://reviews.llvm.org/D13931