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

Eric Liu via cfe-commits cfe-commits at lists.llvm.org
Tue Sep 13 03:47:22 PDT 2016


ioeric added inline comments.

================
Comment at: change-namespace/ChangeNamespace.cpp:359
@@ +358,3 @@
+      End, tok::semi, *Result.SourceManager, Result.Context->getLangOpts(),
+      /*SkipTrailingWhitespaceAndNewLine=*/true);
+  if (AfterSemi.isValid())
----------------
omtcyfz wrote:
> The indentation seems a bit off. Semicolon is on the next line for some reason and I think `/*SkipTrailingWhitespaceAndNewLine=*/` should be `/* SkipTrailingWhitespaceAndNewLine */`.
> 
> UPD: Semicolon came back :)
It seems that Phabricator is not behaving well recently. I saw similar indentation inconsistency in other patches as well.

For the comment, I think either way is fine. You can find both styles in the code base, and I'm not sure which is the standard.
Samples:
https://github.com/llvm-mirror/clang/blob/master/include/clang/Basic/SourceManager.h#L782
https://github.com/llvm-mirror/clang/blob/master/include/clang/Basic/SourceManager.h#L847


https://reviews.llvm.org/D24183





More information about the cfe-commits mailing list