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