hokein added a comment.

I like removing the leading "::" when possible. :)



================
Comment at: change-namespace/ChangeNamespace.cpp:291
+  assert(!SymbolSplitted.empty());
+  SymbolSplitted.pop_back();
+
----------------
Is this needed? Looks like you are removing the name of the symbol here, but 
from the code below, you only use the first element of it.  The QualifiedSymbol 
should always be a fully-qualified name with at least 1 namespace qualifier in 
the code, right?


================
Comment at: change-namespace/ChangeNamespace.cpp:296
+    assert(!NsSplitted.empty());
+    for (auto I = NsSplitted.begin() + 1, E = NsSplitted.end(); I != E; ++I) {
+      if (*I == SymbolSplitted.front())
----------------
Why skipping the first element? And use `is_contained` instead?


================
Comment at: unittests/change-namespace/ChangeNamespaceTests.cpp:718
       "void f() {\n"
-      " auto *ref1 = ::na::A::f;\n"
-      " auto *ref2 = ::na::a_f;\n"
-      " auto *ref3 = ::na::s_f;\n"
+      " auto *ref1 = na::A::f;\n"
+      " auto *ref2 = na::a_f;\n"
----------------
The code style is not correct. Also fix it.


https://reviews.llvm.org/D30493



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to