Page MenuHomePhabricator

A clang tool for changing surrouding namespaces of class/function definitions.
ClosedPublic

Authored by ioeric on Sep 2 2016, 5:11 AM.

Details

Summary

A tool for changing surrouding namespaces of class/function definitions while keeping
references to types in the changed namespace correctly qualified by prepending
namespace specifiers before them.

Example: test.cc

namespace na {                                                          
class X {};                                                             
namespace nb {                                                          
class Y { X x; };                                                       
} // namespace nb                                                       
} // namespace na

To move the definition of class Y from namespace "na::nb" to "x::y", run:

clang-change-namespace --old_namespace "na::nb" \                       
  --new_namespace "x::y" --file_pattern "test.cc" test.cc --

Output:

namespace na {                                                          
class X {};                                                             
} // namespace na                                                       
namespace x {                                                           
namespace y {                                                           
class Y { na::X x; };                                                   
} // namespace y                                                        
} // namespace x

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
hokein added inline comments.Sep 6 2016, 2:16 AM
change-namespace/CMakeLists.txt
9 ↗(On Diff #70308)

I think clangASTMatchers is needed here.

change-namespace/tool/ClangChangeNamespace.cpp
95 ↗(On Diff #70145)

Add a CodeStyle option?

ioeric updated this revision to Diff 70465.Sep 6 2016, 1:37 PM
ioeric marked 10 inline comments as done.
ioeric edited edge metadata.
  • Addressed some review comments.
ioeric added inline comments.Sep 7 2016, 11:17 AM
change-namespace/ChangeNamespace.cpp
21 ↗(On Diff #70308)

Aha, no wonder! Thanks!

480 ↗(On Diff #70308)

Will do.

unittests/change-namespace/ChangeNamespaceTests.cpp
49 ↗(On Diff #70308)

formatAndApplyAllReplacements only formats around replacements. I added format to format the whole file so that I don't need to get every white space right in Code and Expected.

alexshap added inline comments.
change-namespace/ChangeNamespace.cpp
125 ↗(On Diff #70465)

khm, this seems to be a recurring pattern (if i'm not mistaken),
looks like the interface of tooling::Replacements can be changed/improved (although i don't know).
Another (separate) observation is about duplicates. For example for replacements in headers we can get into if(Err) branch pretty frequently because the same replacement can be generated multiple times (if that header is included into several *.cpp files).

alexshap added inline comments.Sep 7 2016, 11:51 AM
change-namespace/tool/ClangChangeNamespace.cpp
48 ↗(On Diff #70465)

probably you need to add cl::Required

cl::opt<std::string> OldNamespace("old_namespace", cl::Required, cl::desc("Old namespace."),
                                                         cl::cat(ChangeNamespaceCategory));

and the same for NewNamespace.

ioeric updated this revision to Diff 70588.Sep 7 2016, 1:04 PM
ioeric marked 9 inline comments as done.
  • Addressed reviewer comments.
ioeric added inline comments.Sep 7 2016, 1:04 PM
change-namespace/ChangeNamespace.cpp
125 ↗(On Diff #70465)

SGTM. An addOrMerge() interface for tooling::Replacements should be helpful.

481 ↗(On Diff #70465)

Added a FallbackStyle.

change-namespace/tool/ClangChangeNamespace.cpp
105 ↗(On Diff #70465)

I'd like to keep this for now for better readability. Will change the format in the future if necessary.

A round of mostly stylistic comments.

change-namespace/ChangeNamespace.cpp
85 ↗(On Diff #70588)

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.

111 ↗(On Diff #70588)

A little confusing wording "a new replacement that is R". Here and later.

124 ↗(On Diff #70588)

(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?

139 ↗(On Diff #70588)

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.

179 ↗(On Diff #70588)

Shouldn't this one be const?

184 ↗(On Diff #70588)

Same for Prefix and Pos.

199 ↗(On Diff #70588)

Probably "} // end namespace " here, at least that's what LLVM Coding Standards suggest.

231 ↗(On Diff #70588)

FIXME without handle here?

307 ↗(On Diff #70588)

const here, too?

335 ↗(On Diff #70588)

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.

347 ↗(On Diff #70588)

const here, too?

362 ↗(On Diff #70588)

const here, too?

381 ↗(On Diff #70588)

Also, if you only use this for an assert, why not just pass Postfix.consume_front(OldNamespace) to assert?

382 ↗(On Diff #70588)

asserts are even more cool when they contain informative message, just like couple dozens of LOCs later.

383 ↗(On Diff #70588)

Totally confused about this line.

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

384 ↗(On Diff #70588)

const here, too?

446 ↗(On Diff #70588)

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.

change-namespace/ChangeNamespace.h
42 ↗(On Diff #70588)

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

change-namespace/tool/ClangChangeNamespace.cpp
84 ↗(On Diff #70588)

I'm not sure whether it actually affects something, but I've enabled C++ and C++17 in clang-rename's options passed somewhere.

Also +R Alex if he has some time to take a look at the code.

ioeric updated this revision to Diff 70793.Sep 9 2016, 2:33 AM
ioeric marked 16 inline comments as done.
  • Addressed reviewer comments.
change-namespace/ChangeNamespace.cpp
85 ↗(On Diff #70588)

Acked.

124 ↗(On Diff #70588)

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.

139 ↗(On Diff #70588)

Acked.

231 ↗(On Diff #70588)

?

335 ↗(On Diff #70588)

There is an example in the comment for the class. Also copied it here :)

381 ↗(On Diff #70588)

I think it is not a good idea to put code with side-effect into assert since assertion can be disabled.

omtcyfz added inline comments.Sep 9 2016, 2:39 AM
change-namespace/ChangeNamespace.cpp
232 ↗(On Diff #70793)

handle == "ioeric" here

s/FIXME(ioeric)/FIXME

336 ↗(On Diff #70793)

Ah, I see. Thank you!

ioeric added subscribers: djasper, klimek.
ioeric updated this revision to Diff 71126.Sep 13 2016, 2:29 AM
ioeric marked 13 inline comments as done.
  • Minor cleanup.
omtcyfz added inline comments.Sep 13 2016, 3:40 AM
change-namespace/ChangeNamespace.cpp
359 ↗(On Diff #71126)

The indentation seems a bit off. Semicolon is on the next line for some reason and I think /*SkipTrailingWhitespaceAndNewLine=*/ should be /* SkipTrailingWhitespaceAndNewLine */.

UPD: Semicolon came back :)

ioeric added inline comments.Sep 13 2016, 3:47 AM
change-namespace/ChangeNamespace.cpp
359 ↗(On Diff #71126)

It seems that Phabricator is not behaving well recently. I saw similar indentation inconsistency in other patches as well.

For the comment, I think either way is fine. You can find both styles in the code base, and I'm not sure which is the standard.
Samples:
https://github.com/llvm-mirror/clang/blob/master/include/clang/Basic/SourceManager.h#L782
https://github.com/llvm-mirror/clang/blob/master/include/clang/Basic/SourceManager.h#L847

hokein added inline comments.Sep 15 2016, 7:12 AM
change-namespace/ChangeNamespace.cpp
200 ↗(On Diff #71126)

Doesn't formatAndApplyAllReplacements format the comments for us?

213 ↗(On Diff #71126)

StringRef.

242 ↗(On Diff #71126)

Actually, hasName matches the name which ends with OldNamespace, which doesn't align with the your comments in OldNamespace member variable.

373 ↗(On Diff #71126)

s/stores/stored

400 ↗(On Diff #71126)

how about assert(Postfix.consume_front(OldNamespace) && "Expect OldNS to start with OldNamespace.");?

421 ↗(On Diff #71126)

Could you add some comments describe what stuff does this function do? The name fixTypeLoc doesn't explain too much. Looks like this function is replacing the qualifiers in TypeLoc.

change-namespace/ChangeNamespace.h
43 ↗(On Diff #71126)

I think you are missing public keyword here, otherwise, it will be private inheritance.

change-namespace/tool/ClangChangeNamespace.cpp
46 ↗(On Diff #71126)

change to "Change namespace"?

106 ↗(On Diff #71126)

Might be add a FIXME?

ioeric updated this revision to Diff 71605.Sep 16 2016, 2:54 AM
ioeric marked 8 inline comments as done.
  • Addressed review comments.
change-namespace/ChangeNamespace.cpp
200 ↗(On Diff #71126)

formatter only adds/removes white spaces I think.

400 ↗(On Diff #71126)

The problem with this is that Postfix.consume_front(OldNamespace) won't be executed if assertion is turned off.

omtcyfz added inline comments.Sep 16 2016, 2:57 AM
change-namespace/ChangeNamespace.cpp
448 ↗(On Diff #71605)

StringRef here too.

ioeric marked an inline comment as done.Sep 16 2016, 3:00 AM
ioeric added inline comments.
change-namespace/ChangeNamespace.cpp
448 ↗(On Diff #71605)

If this was a StringRef, then each map access would require an implicit conversion from StringRef to std::string, which is expensive. And this is already a reference anyway.

omtcyfz added inline comments.Sep 16 2016, 3:02 AM
change-namespace/ChangeNamespace.cpp
448 ↗(On Diff #71605)

Ah, I see. Yes, that's true. Sorry.

hokein accepted this revision.Sep 16 2016, 5:18 AM
hokein edited edge metadata.

The patch looks good to me now.

change-namespace/ChangeNamespace.h
44 ↗(On Diff #71605)

You forgot this one.

101 ↗(On Diff #71605)

Would be clearer to add comment describing the kinds of these replacements (e.g. deleting the forward declarations, replacing the old qualifiers with the new shortest qualified name).

This revision is now accepted and ready to land.Sep 16 2016, 5:18 AM
omtcyfz accepted this revision.Sep 16 2016, 5:25 AM
omtcyfz edited edge metadata.

I have no other objections aswell.
LGTM.

ioeric updated this revision to Diff 71689.Sep 16 2016, 12:43 PM
ioeric marked 4 inline comments as done.
ioeric edited edge metadata.
  • Updated commenting, and fix a bug in the binary.
This revision was automatically updated to reflect the committed changes.
mehdi_amini added inline comments.
clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
396

You shouldn't dereference after a dyn_cast. Either null-check or use cast.

ioeric added inline comments.Nov 10 2016, 10:26 AM
clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
396

Thanks! Fixed by rL286485