Page MenuHomePhabricator

[clang-move] Support moving enum declarations.
ClosedPublic

Authored by hokein on Jan 3 2017, 3:18 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

hokein updated this revision to Diff 82857.Jan 3 2017, 3:18 AM
hokein retitled this revision from to [clang-move] Support moving enum declarations..
hokein updated this object.
hokein added a reviewer: ioeric.
hokein added a subscriber: cfe-commits.
ioeric edited edge metadata.Jan 3 2017, 3:31 AM

Do we consider enum helpers?

clang-move/ClangMove.cpp
171 ↗(On Diff #82857)

These 3 lines seen to be repeated. Maybe pull them out as a MoveTool method?

hokein updated this revision to Diff 82872.Jan 3 2017, 5:20 AM
hokein marked an inline comment as done.
hokein edited edge metadata.

Address review comment.

hokein added a comment.Jan 3 2017, 5:20 AM

Do we consider enum helpers?

This patch excludes enum helpers, only considers the enum declarations which are defined in old.h.
Ideally, we should support enum helpers which are defined in old.cc, but that's another topic.

clang-move/ClangMove.cpp
171 ↗(On Diff #82857)

Done. I made a function for these repeated lines, but it seems hard to figure out a pretty name for it.

ioeric added a comment.Jan 3 2017, 5:35 AM

Do we consider enum helpers?

This patch excludes enum helpers, only considers the enum declarations which are defined in old.h.
Ideally, we should support enum helpers which are defined in old.cc, but that's another topic.

I mean, if a enum helper used in say only old cc, will it be moved? The question is related to D27673. You don't need to support enum helper now, but it would be nice to drop a comment somewhere.

clang-move/ClangMove.h
140 ↗(On Diff #82872)

Maybe just addMovedDecl? move indicates move and delete IMO. But you might need to explain what's really going on in the comment.

hokein updated this revision to Diff 82874.Jan 3 2017, 6:15 AM
hokein marked an inline comment as done.

Refine function name and add a comment for enum helpers.

hokein added a comment.Jan 3 2017, 6:15 AM

I mean, if a enum helper used in say only old cc, will it be moved? The question is related to D27673. You don't need to support enum helper now, but it would be nice to drop a comment somewhere.

Done. Add a comment about this. Enum helpers will not moved whether they are used or not.

clang-move/ClangMove.h
140 ↗(On Diff #82872)

Instead of making an interface in the class, I put this function as a helper function in .cc, and named it as MoveDeclFromOldFileToNewFile. Hope this will make it clearer.

ioeric accepted this revision.Jan 3 2017, 6:17 AM
ioeric edited edge metadata.

Lg

clang-move/ClangMove.cpp
456 ↗(On Diff #82874)

Remove this?

This revision is now accepted and ready to land.Jan 3 2017, 6:17 AM
hokein updated this revision to Diff 82875.Jan 3 2017, 6:21 AM
hokein edited edge metadata.

Remove forgot-deleted code.

hokein updated this revision to Diff 82876.Jan 3 2017, 6:21 AM
hokein marked an inline comment as done.

Not change in ClangMove.h

This revision was automatically updated to reflect the committed changes.