[PATCH] D24183: A clang tool for changing surrouding namespaces of class/function definitions.

[Eric Liu via cfe-commits]

ioeric added inline comments.

================
Comment at: change-namespace/ChangeNamespace.cpp:22
@@ +21,3 @@
+  (void)NS.ltrim(':');
+  return NS.str();
+}
----------------
Yes, it is added to please  `LLVM_ATTRIBUTE_UNUSED_RESULT` of `llvm::StringRef::ltrim`.

================
Comment at: change-namespace/ChangeNamespace.cpp:30
@@ +29,3 @@
+  std::string Result = Namespaces.front();
+  for (auto I = Namespaces.begin() + 1, E = Namespaces.end(); I != E; ++I) {
+    Result += ("::" + *I).str();
----------------
omtcyfz wrote:
> Braces around single-line statement inside control flow statement body is occasionally not welcome in LLVM Codebase.
> 
> There are actually both many braces around single-line statement inside cfs body here and many cfs's without braces around single-line body inside a single file, which isn't good in terms of consistency at least inside the project.
You are right! I missed those braces when converting the code from google style :P Thanks for the catch!

================
Comment at: change-namespace/ChangeNamespace.cpp:105
@@ +104,3 @@
+  llvm::StringRef File = SM.getBufferData(LocInfo.first, &InvalidTemp);
+  if (InvalidTemp)
+    return SourceLocation();
----------------
omtcyfz wrote:
> Shouldn't this throw an error instead?
An empty `SourceLocation` is invalid and often used to indicate error. Added error handling at callers.


https://reviews.llvm.org/D24183