This is an archive of the discontinued LLVM Phabricator instance.

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

Event Timeline

ioeric updated this revision to Diff 70144.Sep 2 2016, 5:11 AM
ioeric retitled this revision from to A clang tool for changing surrouding namespaces of class/function definitions..
ioeric updated this object.
ioeric updated this object.Sep 2 2016, 5:17 AM
ioeric updated this object.
ioeric updated this object.
ioeric updated this revision to Diff 70145.Sep 2 2016, 5:32 AM
ioeric updated this object.
  • Added comments and cleanup.
ioeric added a subscriber: cfe-commits.
ioeric updated this revision to Diff 70155.Sep 2 2016, 6:52 AM
  • Minor cleanup.
Eugene.Zelenko set the repository for this revision to rL LLVM.
Eugene.Zelenko added a subscriber: Eugene.Zelenko.

Looks like clang-refactor idea should finally go live.

omtcyfz edited edge metadata.Sep 2 2016, 7:30 AM

Looks like clang-refactor idea should finally go live.

D24192 up for review.

omtcyfz added inline comments.Sep 2 2016, 7:46 AM
change-namespace/ChangeNamespace.cpp
21

is (void) intended here?

change-namespace/ChangeNamespace.h
41

FIXME?

ioeric updated this revision to Diff 70173.Sep 2 2016, 9:17 AM
ioeric edited edge metadata.
  • removed unintended changes.
omtcyfz added inline comments.Sep 4 2016, 5:21 PM
change-namespace/ChangeNamespace.cpp
31

Braces around single-line statement inside control flow statement body is occasionally not welcome in LLVM Codebase.

There are actually both many braces around single-line statement inside cfs body here and many cfs's without braces around single-line body inside a single file, which isn't good in terms of consistency at least inside the project.

106

Shouldn't this throw an error instead?

ioeric updated this revision to Diff 70308.Sep 5 2016, 1:18 AM
ioeric marked 4 inline comments as done.
  • Addressed review comments.
change-namespace/ChangeNamespace.cpp
23

Yes, it is added to please LLVM_ATTRIBUTE_UNUSED_RESULT of llvm::StringRef::ltrim.

31

You are right! I missed those braces when converting the code from google style :P Thanks for the catch!

106

An empty SourceLocation is invalid and often used to indicate error. Added error handling at callers.

omtcyfz added inline comments.Sep 5 2016, 1:46 AM
change-namespace/ChangeNamespace.cpp
481

By the way, shouldn't this one be "LLVM"?

omtcyfz added inline comments.Sep 5 2016, 5:19 AM
change-namespace/ChangeNamespace.cpp
481

Alternatively, it might be nice to provide an option to specify desired formatting style.

hokein edited edge metadata.Sep 6 2016, 2:16 AM

some initial comments.

change-namespace/ChangeNamespace.cpp
22

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.

142

StringRef?

215

Use StringRef.

481

+1 on adding a CodeStyle command-line option.

change-namespace/ChangeNamespace.h
68

The first letter should be lower case.

71

The same.

change-namespace/tool/ClangChangeNamespace.cpp
105

Instead of printing results in a customized format, it makes more sense to print it in a JSON format, like:

[
   { "FilePath": "XXX"
     "SourceText": "YYYY"}
]
test/change-namespace/simple-move.cpp
8

Usually, we put the test scripts at top of the test file, and use CHECK-NEXT is more suitable in your case here?

unittests/change-namespace/ChangeNamespaceTests.cpp
50

Looks like formatAndApplyAllReplacements has already formatted the code, why do we need to format it again?

hokein added inline comments.Sep 6 2016, 2:16 AM
change-namespace/CMakeLists.txt
10

I think clangASTMatchers is needed here.

change-namespace/tool/ClangChangeNamespace.cpp
96

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
22

Aha, no wonder! Thanks!

481

Will do.

unittests/change-namespace/ChangeNamespaceTests.cpp
50

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.

alexander-shaposhnikov added inline comments.
change-namespace/ChangeNamespace.cpp
126

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

change-namespace/tool/ClangChangeNamespace.cpp
49

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
126

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

482

Added a FallbackStyle.

change-namespace/tool/ClangChangeNamespace.cpp
106

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
86

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.

112

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

125

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

140

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.

180

Shouldn't this one be const?

185

Same for Prefix and Pos.

200

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

232

FIXME without handle here?

308

const here, too?

336

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.

348

const here, too?

363

const here, too?

382

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

383

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

384

Totally confused about this line.

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

385

const here, too?

447

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
43

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
85

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
86

Acked.

125

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.

140

Acked.

232

?

336

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

382

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
233

handle == "ioeric" here

s/FIXME(ioeric)/FIXME

337

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
360

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
360

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
201

Doesn't formatAndApplyAllReplacements format the comments for us?

214

StringRef.

243

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

374

s/stores/stored

401

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

422

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
44

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

change-namespace/tool/ClangChangeNamespace.cpp
47

change to "Change namespace"?

107

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
201

formatter only adds/removes white spaces I think.

401

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
449

StringRef here too.

ioeric marked an inline comment as done.Sep 16 2016, 3:00 AM
ioeric added inline comments.
change-namespace/ChangeNamespace.cpp
449

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
449

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
45

You forgot this one.

102

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 ↗(On Diff #71848)

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 ↗(On Diff #71848)

Thanks! Fixed by rL286485