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

[Haojian Wu via cfe-commits]

hokein added a comment.

some initial comments.


================
Comment at: change-namespace/CMakeLists.txt:9
@@ +8,3 @@
+  LINK_LIBS
+  clangAST
+  clangBasic
----------------
I think `clangASTMatchers` is needed here.

================
Comment at: change-namespace/ChangeNamespace.cpp:21
@@ +20,3 @@
+inline std::string formatNamespace(llvm::StringRef NS) {
+  (void)NS.ltrim(':');
+  return NS.str();
----------------
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.

================
Comment at: change-namespace/ChangeNamespace.cpp:141
@@ +140,3 @@
+tooling::Replacement createReplacement(SourceLocation Start, SourceLocation End,
+                                       const std::string &ReplacementText,
+                                       const SourceManager &SM) {
----------------
StringRef?

================
Comment at: change-namespace/ChangeNamespace.cpp:214
@@ +213,3 @@
+ChangeNamespaceTool::ChangeNamespaceTool(
+    const std::string &OldNs, const std::string &NewNs,
+    const std::string &FilePattern,
----------------
Use `StringRef`.

================
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.
----------------
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.

================
Comment at: change-namespace/ChangeNamespace.h:67
@@ +66,3 @@
+
+  void FixUsingShadowDecl(const ast_matchers::MatchFinder::MatchResult &Result,
+                          const UsingDecl *UsingDecl);
----------------
The first letter should be lower case.

================
Comment at: change-namespace/ChangeNamespace.h:70
@@ +69,3 @@
+
+  void ReplaceQualifiedSymbolInDeclContext(
+      const ast_matchers::MatchFinder::MatchResult &Result,
----------------
The same.

================
Comment at: change-namespace/tool/ClangChangeNamespace.cpp:95
@@ +94,3 @@
+
+  if (!formatAndApplyAllReplacements(Tool.getReplacements(), Rewrite)) {
+    llvm::errs() << "Failed applying all replacements.\n";
----------------
Add a `CodeStyle` option?

================
Comment at: change-namespace/tool/ClangChangeNamespace.cpp:104
@@ +103,3 @@
+    auto ID = Sources.translateFile(Entry);
+    outs() << "============== " << File << " ==============\n";
+    Rewrite.getEditBuffer(ID).write(llvm::outs());
----------------
Instead of printing results in a customized format, it makes more sense to print it in a JSON format, like:

```
[
   { "FilePath": "XXX"
     "SourceText": "YYYY"}
]
```

================
Comment at: test/change-namespace/simple-move.cpp:7
@@ +6,2 @@
+
+// RUN: clang-change-namespace -old_namespace "na::nb" -new_namespace "x::y" --file_pattern ".*" %s -- | sed 's,// CHECK.*,,' | FileCheck %s
----------------
Usually, we put the test scripts at top of the test file, and use `CHECK-NEXT` is more suitable in your case here?

================
Comment at: unittests/change-namespace/ChangeNamespaceTests.cpp:49
@@ +48,3 @@
+    formatAndApplyAllReplacements(FileToReplacements, Context.Rewrite);
+    return format(Context.getRewrittenText(ID));
+  }
----------------
Looks like `formatAndApplyAllReplacements` has already formatted the code, why do we need to format it again?


https://reviews.llvm.org/D24183