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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
