[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:21
@@ +20,3 @@
+inline std::string formatNamespace(llvm::StringRef NS) {
+  (void)NS.ltrim(':');
+  return NS.str();
----------------
hokein wrote:
> this statement doesn't do the intended thing (It won't change `NS`). The result returned by `ltrim` is what you want here, I think.
Aha,  no wonder! Thanks!

================
Comment at: change-namespace/ChangeNamespace.cpp:480
@@ +479,3 @@
+    Replaces = Replaces.merge(NewReplacements);
+    format::FormatStyle Style = format::getStyle("file", FilePath, "google");
+    // Clean up old namespaces if there is nothing in it after moving.
----------------
hokein wrote:
> omtcyfz wrote:
> > omtcyfz wrote:
> > > By the way, shouldn't this one be "LLVM"?
> > Alternatively, it might be nice to provide an option to specify desired formatting style.
> +1 on adding a `CodeStyle` command-line option.
Will do.

================
Comment at: unittests/change-namespace/ChangeNamespaceTests.cpp:49
@@ +48,3 @@
+    formatAndApplyAllReplacements(FileToReplacements, Context.Rewrite);
+    return format(Context.getRewrittenText(ID));
+  }
----------------
hokein wrote:
> Looks like `formatAndApplyAllReplacements` has already formatted the code, why do we need to format it again?
`formatAndApplyAllReplacements ` only formats around replacements. I added `format` to format the whole file so that I don't need to get every white  space right in `Code` and `Expected`.


https://reviews.llvm.org/D24183