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