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

[Kirill Bobyrev via cfe-commits]

omtcyfz added a comment.

A round of mostly stylistic comments.


================
Comment at: change-namespace/ChangeNamespace.cpp:85
@@ +84,3 @@
+
+SourceLocation getStartOfNextLine(SourceLocation Loc, const SourceManager &SM,
+                                  const LangOptions &LangOpts) {
----------------
Wouldn't it be better of in some common interface?

A couple of useful cases for other refactorings come to my mind. I.e. `getStartOfPreviousLine` would be nice to have there, too.

For this patch, though, I think `FIXME` would be enough.

================
Comment at: change-namespace/ChangeNamespace.cpp:111
@@ +110,3 @@
+
+// Returns a new replacement that is `R` with range that refers to
+// code after `Replaces` being applied.
----------------
A little confusing wording "a new replacement that is `R`". Here and later.

================
Comment at: change-namespace/ChangeNamespace.cpp:124
@@ +123,3 @@
+// Adds a replacement `R` into `Replaces` or merges it into `Replaces` by
+// applying all existing Replaces first if there is conflict.
+void addOrMergeReplacement(const tooling::Replacement &R,
----------------
(probably not very much related to the patch and more out of curiosity)

If there is a conflict, why does it get resolved by applying all existing `Replaces` first?

================
Comment at: change-namespace/ChangeNamespace.cpp:139
@@ +138,3 @@
+  if (!Start.isValid() || !End.isValid()) {
+    llvm::errs() << "start or end location were invalid\n";
+    return tooling::Replacement();
----------------
Alex suggested using diagnostics instead of dumping stuff to `llvm::errs()` in D24224 and I think it looks way better. Actually, you can output locations with that, which makes error message more informative. It might be also easier to track down bugs with that info, rather than with raw error messages.

That's just a nit, probably `FIXME` is enough.

Same comment applies to the other `llvm::errs()` usages in this function.

================
Comment at: change-namespace/ChangeNamespace.cpp:179
@@ +178,3 @@
+    return DeclName;
+  auto UnqualifiedName = DeclNameSplitted.back();
+  while (true) {
----------------
Shouldn't this one be `const`?

================
Comment at: change-namespace/ChangeNamespace.cpp:184
@@ +183,3 @@
+      return DeclName;
+    auto Prefix = NsName.substr(0, Pos - 1);
+    if (DeclName.startswith(Prefix))
----------------
Same for `Prefix` and `Pos`.

================
Comment at: change-namespace/ChangeNamespace.cpp:199
@@ +198,3 @@
+    Code = ("namespace " + NsSplitted.back() + " {\n" + Code +
+            "} // namespace " + NsSplitted.back() + "\n")
+               .str();
----------------
Probably `"} // end namespace "` here, at least that's what [[ http://llvm.org/docs/CodingStandards.html#namespace-indentation | LLVM Coding Standards ]] suggest.

================
Comment at: change-namespace/ChangeNamespace.cpp:231
@@ +230,3 @@
+
+// FIXME(ioeric): handle the following symbols:
+//   - Types in `UsingShadowDecl` (e.g. `using a::b::c;`) which are not matched
----------------
`FIXME` without handle here?

================
Comment at: change-namespace/ChangeNamespace.cpp:307
@@ +306,3 @@
+  // retrieving offset and length from it.
+  auto R = createReplacement(Start, End, "", *Result.SourceManager);
+  MoveNamespace MoveNs;
----------------
`const` here, too?

================
Comment at: change-namespace/ChangeNamespace.cpp:335
@@ +334,3 @@
+// into the old namespace after moving code from the old namespace to the new
+// namespace.
+void ChangeNamespaceTool::moveClassForwardDeclaration(
----------------
Slightly strange wording. I recall an offline discussion where you told about it, so I can understand that, but probably an example would be cool to have.

================
Comment at: change-namespace/ChangeNamespace.cpp:347
@@ +346,3 @@
+  // Delete the forward declaration from the code to be moved.
+  auto Deletion = createReplacement(Start, End, "", *Result.SourceManager);
+  addOrMergeReplacement(Deletion, &FileToReplacements[Deletion.getFilePath()]);
----------------
`const` here, too?

================
Comment at: change-namespace/ChangeNamespace.cpp:362
@@ +361,3 @@
+  assert(!NsDecl->decls_empty());
+  auto Insertion = createInsertion(NsDecl->decls_begin()->getLocStart(), Code,
+                                   *Result.SourceManager);
----------------
`const` here, too?

================
Comment at: change-namespace/ChangeNamespace.cpp:381
@@ +380,3 @@
+  llvm::StringRef Postfix = OldNs;
+  bool Consumed = Postfix.consume_front(OldNamespace);
+  assert(Consumed);
----------------
Also, if you only use this for an assert, why not just pass `Postfix.consume_front(OldNamespace)` to `assert`?

================
Comment at: change-namespace/ChangeNamespace.cpp:382
@@ +381,3 @@
+  bool Consumed = Postfix.consume_front(OldNamespace);
+  assert(Consumed);
+  (void)Consumed;
----------------
`assert`s are even more cool when they contain informative message, just like couple dozens of LOCs later.

================
Comment at: change-namespace/ChangeNamespace.cpp:383
@@ +382,3 @@
+  assert(Consumed);
+  (void)Consumed;
+  std::string NewNs = (NewNamespace + Postfix).str();
----------------
Totally confused about this line.

UPD: Ah, I see. You don't use `Consumed` anywhere else :) See comment above.

================
Comment at: change-namespace/ChangeNamespace.cpp:384
@@ +383,3 @@
+  (void)Consumed;
+  std::string NewNs = (NewNamespace + Postfix).str();
+
----------------
`const` here, too?

================
Comment at: change-namespace/ChangeNamespace.cpp:446
@@ +445,3 @@
+      // code.
+      unsigned NewOffset = Replaces.getShiftedCodePosition(NsMove.Offset);
+      unsigned NewLength =
----------------
I know, I know...

`const` here and everywhere else, too?

Maybe just running some clang-tidy check would do that. If we don't have one, I would file a feature request.

================
Comment at: change-namespace/ChangeNamespace.h:42
@@ +41,3 @@
+//   }  // x
+// FIXME: support moving typedef, enums across namespaces.
+class ChangeNamespaceTool : ast_matchers::MatchFinder::MatchCallback {
----------------
Also, I'm not sure how to do that with gTest, but XFAIL tests are always nice in terms of an illustration what the tool still can't do (at least for me).

================
Comment at: change-namespace/tool/ClangChangeNamespace.cpp:84
@@ +83,3 @@
+    return Result;
+  LangOptions DefaultLangOptions;
+  IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts = new DiagnosticOptions();
----------------
I'm not sure whether it actually affects something, but I've enabled C++ and C++17 in `clang-rename`'s options passed somewhere.


https://reviews.llvm.org/D24183