This is an archive of the discontinued LLVM Phabricator instance.

[clang-move] A prototype tool for moving class definition to new file.
ClosedPublic

Authored by hokein on Sep 6 2016, 1:50 AM.

Details

Summary

This patch introduces a new tool which moves a specific class definition
from files (.h, .cc) to new files (.h, .cc), which mostly acts like
"Extract class defintion". In the long term, this tool should be
merged in to clang-refactoring as a subtool.

clang-move not only moves class definition, but also moves all the
forward declarations, functions defined in anonymous namespace and #include
headers to new files, to make sure the new files are compliable as much
as possible.

To move Foo from old.[h/cc] to new.[h/cc], use:

clang-move -name=Foo -old_header=old.h -old_cc=old.cc -new_header=new.h
-new_cc=new.cc old.cc

To move Foo from old.h to new.h, use:

clang-move -name=Foo -old_header=old.h -new_header=new.h old.cc

Diff Detail

Repository
rL LLVM

Event Timeline

hokein updated this revision to Diff 70361.Sep 6 2016, 1:50 AM
hokein retitled this revision from to [clang-move] A prototype tool for moving class definition to new file..
hokein updated this object.
hokein added a subscriber: cfe-commits.
hokein updated this revision to Diff 70362.Sep 6 2016, 1:54 AM

Minor cleanup.

hokein added subscribers: omtcyfz, bkramer.
hokein updated this revision to Diff 70363.Sep 6 2016, 2:05 AM

Fix function name style.

omtcyfz added inline comments.Sep 6 2016, 2:07 AM
clang-move/ClangMove.h
25 ↗(On Diff #70363)

FIXME?

Eugene.Zelenko added inline comments.
clang-move/ClangMove.h
33 ↗(On Diff #70363)

Please add empty line before. Why don't use C++11 members initialization and = default?

83 ↗(On Diff #70363)

Is explicit necessary?

106 ↗(On Diff #70363)

Please add empty line before.

Eugene.Zelenko added inline comments.Sep 6 2016, 12:03 PM
clang-move/ClangMove.h
13 ↗(On Diff #70363)

Isn't C++ headers should be after Clang headers?

Probably old_source/new_source will be better, because different extensions are used for C++ files.

ioeric edited edge metadata.Sep 6 2016, 1:11 PM

NIce! Some initial comments.

clang-move/ClangMove.cpp
38 ↗(On Diff #70363)

Might want to comment on how #include "old_header" is handled here?

39 ↗(On Diff #70363)

Braces not needed.

103 ↗(On Diff #70363)

Consider the code below to also include trailing comment.

clang::SourceLocation after_semi = clang::Lexer::findLocationAfterToken(
    D->getLocEnd, clang::tok::semi, *SM,
    result.Context->getLangOpts(),
    /*SkipTrailingWhitespaceAndNewLine=*/true);
113 ↗(On Diff #70363)

Intuitively, it would be easier and more efficient if you construct the code as a string and then insert the code with one replacement.

119 ↗(On Diff #70363)

FIXME: add header guard for new header file.

125 ↗(On Diff #70363)

If two declarations in the same namespace are not consecutive in the vector (i.e. there is a decl in a different ns between them), will they be put into two canonical namespace blocks?

129 ↗(On Diff #70363)

Is it possible to restrict all MovedDecl.Decl to NamedDecl so that we can use getQualifiedNameAsString instead of having a second implementation of retrieving namespaces.

Also how is anonymous namespace handled here?

157 ↗(On Diff #70363)

FIXME: also consider comments for the declaration.

199 ↗(On Diff #70363)

What about static functions?

210 ↗(On Diff #70363)

FIXME: support out-of-line member variable initializations etc? Or is it already covered?

240 ↗(On Diff #70363)

No braces.

Also, I am worrying if endswith is a safe way to compare files. For example, what if FileName is relative path?

254 ↗(On Diff #70363)

nit: RemovedReplacement sounds like the replacement itself is being removed. RemoveReplacement would be better IMO.

clang-move/ClangMove.h
39 ↗(On Diff #70363)

Should the Name be fully qualified?

52 ↗(On Diff #70363)

Variable style. Here and some more below.

59 ↗(On Diff #70363)

A comment for what this function does would be appreciated.

65 ↗(On Diff #70363)

Is there any reason why you made this a reference?

clang-move/tool/ClangMoveMain.cpp
102 ↗(On Diff #70363)

Indentation.

hokein updated this revision to Diff 70538.Sep 7 2016, 8:26 AM
hokein marked 19 inline comments as done.
hokein edited edge metadata.
  • Address review comments.
hokein added inline comments.Sep 7 2016, 8:27 AM
clang-move/ClangMove.cpp
38 ↗(On Diff #70363)

Currently the old_header is also copied to new file.

103 ↗(On Diff #70363)

But this code could not handle cases where the declaration definition has no semi at the end, such as "void f() {}"

125 ↗(On Diff #70363)

Yes, only declarations which are consecutive in the vector and in the same namespace are put in one namespace block.

129 ↗(On Diff #70363)

Yeah, getQualifiedNameAsString looks like a simpler way, but it doesn't suitable for our usage here.

The getQualifiedNameAsString includes the name of the class. For instance, a class method decl like void A::f() {} defined in global namespace, it retruns A::f which is not intended.

It handles anonymous namespace by wrapping like namespace { ... } // namespace, see the test.

199 ↗(On Diff #70363)

Good catch, I missed it. Added.

210 ↗(On Diff #70363)

Add support now.

240 ↗(On Diff #70363)

The FileName is from the PPCallbacks::InclusionDirective APIs. From my experience, it seems always to be an absolute file path either I pass a relative file path or a absolute path to clang-move, but this is no guarantee in the document.

Using endswith is the most reliable way so far.

clang-move/ClangMove.h
39 ↗(On Diff #70363)

It's not required. The name can be fully/partially qualified, e.g., ::a::b::X, a::b::X, b::X, X, which has the same behavior with hasName ast matcher.

It the given name is partially qualified, it will match all the class whose name ends with the given name.

65 ↗(On Diff #70363)

To avoid the cost of making a copy of the structure.

83 ↗(On Diff #70363)

The explicit keyword can prevent the copy-list-initialization like ClangMoveAction a = {spec, file_to_replacement}. I'm fine to remove it since it is trival.

clang-move/tool/ClangMoveMain.cpp
102 ↗(On Diff #70363)

Indentation is correct here, clang-format doesn't complain anything about it.

ioeric added inline comments.Sep 7 2016, 1:23 PM
clang-move/ClangMove.cpp
103 ↗(On Diff #70363)

Please see the comment for findLocationAfterToken.

If the token is not found or the location is inside a macro, the returned source location will be invalid.

129 ↗(On Diff #70363)

I think it should be relatively easy to split the qualified name returned by getQualifiedNameAsString .

It handles anonymous namespace by wrapping like namespace { ... } // namespace, see the test.

I mean...wouldn't getNamespaces return something like "a::(anonymous)" for anonymous namespace? How do you handle this?

clang-move/ClangMove.h
39 ↗(On Diff #70363)

It the given name is partially qualified, it will match all the class whose name ends with the given name.

In this case, all matched classes will be moved right? Might want to mention this in the comment.

65 ↗(On Diff #70363)

If the copy is not that expensive, I'd just make a copy so that I don't need to worry about the life-cycle of Spec.

hokein updated this revision to Diff 71506.Sep 15 2016, 8:09 AM
hokein marked 2 inline comments as done.

Address review comments.

hokein added inline comments.Sep 15 2016, 8:10 AM
clang-move/ClangMove.cpp
104 ↗(On Diff #70538)

Aha, I see. I misused the findLocationAfterToken previously.

130 ↗(On Diff #70538)

For anonymous namespace, getNamespaces just makes it an empty string.
Apart from that, we have to deal with other exceptions, for instance:

namespace a {
void A::f() {}
}

What we want is namespace a, but getQualifiedNameAsString returns a::A::f. Thus, using getQualifiedNameAsString wouldn't simplify the code at the moment.

ioeric accepted this revision.Sep 16 2016, 1:28 AM
ioeric edited edge metadata.

lg

unittests/clang-move/ClangMoveTests.cpp
122 ↗(On Diff #71506)

It's fine for this patch, but I think we also want to add newlines be between moved declarations, especially if there were newlines in the original code.

This revision is now accepted and ready to land.Sep 16 2016, 1:28 AM
hokein updated this revision to Diff 71631.Sep 16 2016, 6:30 AM
hokein edited edge metadata.

Support fully quailified name only.

hokein updated this revision to Diff 72027.Sep 21 2016, 5:14 AM

Update dependency.

hokein updated this revision to Diff 72031.Sep 21 2016, 6:07 AM

Improve the way of detecting whether #include in old_header.

This revision was automatically updated to reflect the committed changes.

Maybe it could be mentioned in the release notes?

Herald added a project: Restricted Project. · View Herald TranscriptOct 4 2019, 11:37 AM

Maybe it could be mentioned in the release notes?

Forget about this comment, I have been distracted by the fact that it is now installed
https://reviews.llvm.org/D68423