Page MenuHomePhabricator

[migrate-tool] Framework for a codebase-dependent migration tool.
AbandonedPublic

Authored by ioeric on Sep 9 2016, 2:06 AM.

Details

Summary

This tool aims to automates the entire process of migrating a C++ symbol (class,
function, enum etc.) including renaming a symbol and/or moving a symbol
across namespace and/or files. Besides moving the symbol definition, it also
takes care of users of the symbol (i.e. update all references of old symbol
to new symbol) as well as build rules.
The migration path is divided into 3 phases:

  1. Preparation phase. Create an alias from the old symbol name to to new symbol name. If the symbol is to be moved acrossed files, a new temporary header containing type alias is created.
  2. Renaming phase. Update all affected projects (i.e. users of the old symbol) to use the new symbol name via the alias above.
  3. Migration phase. Actual migration happens at this phase after all affected projects are updated. Move the symbol definition to the new file if the symbol is to be moved to a different file. Rename the symbol (i.e. the definition) if the symbol is to be renamed. Change the namespace in which the symbol is defined if the symbol is to be moved to a different namespace.

With this migration path, we can divide the three phases into three separate
patches. For large scale changes where many projects are affected, the
renaming phase can be splitted into multiple smaller patches to make the
both refactoring and review process easier.
clean make rules

Event Timeline

ioeric updated this revision to Diff 70792.Sep 9 2016, 2:06 AM
ioeric retitled this revision from to [migrate-tool] Framework for a codebase-dependent migration tool..
ioeric updated this object.
ioeric added a subscriber: cfe-commits.

I think will be good idea to await clang-refactor and merge code there.

Please run Include What You Use. Code use a lot of containers and will be good to include them explicitly.

migrate-tool/HeaderBuild.cpp
28 ↗(On Diff #70792)

Please separate with empty line.

I think will be good idea to await clang-refactor and merge code there.

This tool is not exactly a clang-tool; it is a framework that invokes multiple clang refactoring tool and also manipulates build rules. Also, I don't think two on-going projects should await on one another. If this tool fits into clang-refactor when it's actually there, I'd be happy to merge it into clang-refactor.

Please run Include What You Use. Code use a lot of containers and will be good to include them explicitly.

Thanks for the tip!

omtcyfz edited edge metadata.Sep 9 2016, 1:03 PM

I think will be good idea to await clang-refactor and merge code there.

This tool is not exactly a clang-tool; it is a framework that invokes multiple clang refactoring tool and also manipulates build rules. Also, I don't think two on-going projects should await on one another. If this tool fits into clang-refactor when it's actually there, I'd be happy to merge it into clang-refactor.

+1 on that. It's fine to push standalone tools first.

One round of llvm-API specifics.

migrate-tool/BuildManager.h
23

These methods could use some documentation.

migrate-tool/HeaderBuild.cpp
19 ↗(On Diff #70792)

There's llvm::join for this.

30 ↗(On Diff #70792)

Is (Name != "" && IsTopLevel == true) a valid input for this? If not, add an assert.

34 ↗(On Diff #70792)

Use raw_string_ostream instead.

103 ↗(On Diff #70792)

NewNameSplitted.back()

migrate-tool/MigrateTool.cpp
27

Use llvm::raw_fd_ostream.

80

return QualifiedName.rsplit(':').second;

migrate-tool/MigrateTool.h
86

createFile? writeFile? add sounds like it adds something to the tool, which it doesn't.

migrate-tool/RefactorManager.h
24 ↗(On Diff #70792)

Documentation for the methods? :)

31 ↗(On Diff #70792)

Here and below, prefer llvm::ArrayRef over const std::vector&/const SmallVectorImpl &. It's agnostic of the underlying vector implementation and gives you an immutable view.

klimek added inline comments.Sep 12 2016, 2:14 AM
migrate-tool/BuildManager.h
23

I'm not sure the header-only distinction makes sense.
I'd start with
virtual bool addLibrary(llvm::ArrayRef<std::string> Sources) = 0;
or similar.

Also, needs more comments :) Especially what this would mean for different build systems.

28

I think we'll want a more precise name, as this might mean different things (target that compiles File vs target that outputs File, etc).
What do you want this to do?

migrate-tool/HeaderBuild.cpp
19 ↗(On Diff #70792)

Can't we use join from ADT/StringExtras.h?

migrate-tool/HeaderBuild.h
19 ↗(On Diff #70792)

I'd probably call this HeaderGenerator or something.

29 ↗(On Diff #70792)

This all needs more comments :)
I assume we'll also need to somehow give this a "style" at some point? Or alternatively create dumb data structures, and have a style interface that can create the actual source according to a style from the data we provide?

migrate-tool/RefactorManager.h
24 ↗(On Diff #70792)

I think "RefactoringManager" reads better; otherwise this sounds like you want to refactor the manager :)

That said, I think we'll need to split this - getAffectedFiles belongs in its own interface, and we might want to have a common interface with what Kirill and Benjamin are working on.

ioeric updated this revision to Diff 71116.Sep 13 2016, 1:39 AM
ioeric marked 13 inline comments as done.
ioeric edited edge metadata.
  • Addressed review comments.
  • Separate getAffectedFiles into its own interface.
ioeric added inline comments.Sep 13 2016, 1:39 AM
migrate-tool/HeaderBuild.h
29 ↗(On Diff #70792)

Will add style support when necessary. Added FIXME for now.

ioeric edited reviewers, added: bkramer; removed: omtcyfz, djasper.Sep 15 2016, 5:15 AM
ioeric added subscribers: omtcyfz, djasper.

Ping

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

Sorry for the delay.
The patch only contains an unittest for HeaderGenerator, which is not quite enough. Should we create a fake migrate-tool binary to illustrate APIs usage?

migrate-tool/HeaderGenerator.h
23

explicit.

migrate-tool/MigrateTool.cpp
100

s/send/end

migrate-tool/MigrateTool.h
51

What is the 'MigrateType' used for? I can't find any usage in the patch.

migrate-tool/RefactoringManager.h
29

The arguments llvm::StringRef and std::string look inconsistent to me at first glance. But currently it is fine, we can modify it if needed in the future.

ioeric updated this revision to Diff 71845.Sep 19 2016, 10:31 AM
ioeric marked 4 inline comments as done.
ioeric edited edge metadata.
  • Added MigrationEnvironment class and a dummy migrate-tool binary.

Sorry for the delay.
The patch only contains an unittest for HeaderGenerator, which is not quite enough. Should we create a fake migrate-tool binary to illustrate APIs usage?

Done.

Good idea. I wanted to add a dummy tool once the interfaces are stable. But I guess now is the right time.

migrate-tool/MigrateTool.h
51

Deleted it for now. Will add it when it is actually used in the future.

The interfaces look good to me roughly.

migrate-tool/MigrationEnvironment.h
23

MigrationContext might be better?

25

This is not needed IMO since you have defined a four-parameters constructor below, compiler won't generate the default constructor implicitly.

klimek added inline comments.Sep 26 2016, 1:03 AM
migrate-tool/AffectedFilesFinder.h
25–26

Comment a bit on what the contract here is: will these be absolute or relative paths?

migrate-tool/BuildManager.h
29

I think this needs more comments explaining what BuildTarget / Dependency will be. Similarly for Name above.

migrate-tool/DummyMigrationEnvironment.h
18–20 ↗(On Diff #71845)

Do we perhaps want to create fakes here, where we can later query the libraries / dependencies that exist?

ioeric updated this revision to Diff 72635.Sep 27 2016, 5:54 AM
ioeric marked 2 inline comments as done.
  • Replace dummy binary with unit test with dummy environemnt + fake codebase.
klimek added inline comments.Sep 30 2016, 1:57 AM
migrate-tool/MigrateTool.cpp
53

I'm wondering whether we really want to evolve this to a callback / executor based design. That is, have AffectedFilesFinder give an interface like RunOnAffectedFiles(...)

ioeric added inline comments.Sep 30 2016, 2:21 AM
migrate-tool/MigrateTool.cpp
53

That sounds like a good design, but I'm not sure if we need this at this point since actions on affected files would simply be renaming and include fixing now and in the foreseeable future.

klimek added inline comments.Sep 30 2016, 2:25 AM
migrate-tool/MigrateTool.cpp
53

Well, my main concern is how we hook this up to full codebase wide analyses. Currently, we basically hard-code the execution strategy, right?

ioeric added inline comments.Sep 30 2016, 2:34 AM
migrate-tool/MigrateTool.cpp
53

I see. That makes sense then. Thanks!

ioeric updated this revision to Diff 73446.Oct 4 2016, 3:34 AM
  • Add SymbolRenameSpec; replace addIncludesToFiles and renameSymbolsInFiles with renameSymbolsInAffectedFiles.
ioeric added a comment.Oct 6 2016, 6:19 AM

Test phab...sorry spamming...

ioeric abandoned this revision.Dec 19 2017, 5:53 AM