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

[Eric Liu via cfe-commits]

ioeric added inline comments.

================
Comment at: change-namespace/ChangeNamespace.cpp:85
@@ +84,3 @@
+
+SourceLocation getStartOfNextLine(SourceLocation Loc, const SourceManager &SM,
+                                  const LangOptions &LangOpts) {
----------------
omtcyfz wrote:
> 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.
Acked.

================
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,
----------------
omtcyfz wrote:
> (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?
It's a bit complicated to explain...Please see the comment for `tooling::Replacements`, specifically the `merge` function. Generally, the idea is that conflicting replacements will be merged into a single one so that the set of replacements never conflict.

================
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();
----------------
omtcyfz wrote:
> 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.
Acked.

================
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
----------------
omtcyfz wrote:
> `FIXME` without handle here?
?

================
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(
----------------
omtcyfz wrote:
> 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.
There is an example in the comment for the class. Also copied it here :)

================
Comment at: change-namespace/ChangeNamespace.cpp:381
@@ +380,3 @@
+  llvm::StringRef Postfix = OldNs;
+  bool Consumed = Postfix.consume_front(OldNamespace);
+  assert(Consumed);
----------------
omtcyfz wrote:
> Also, if you only use this for an assert, why not just pass `Postfix.consume_front(OldNamespace)` to `assert`?
I think it is not a good idea to put code with side-effect into `assert` since assertion can be disabled.


https://reviews.llvm.org/D24183