[PATCH] D24380: [migrate-tool] Framework for a codebase-dependent migration tool.

[Manuel Klimek via cfe-commits]

klimek added inline comments.

================
Comment at: migrate-tool/BuildManager.h:22
@@ +21,3 @@
+public:
+  virtual bool addHeaderOnlyLibrary(llvm::StringRef HeaderPath) = 0;
+
----------------
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.

================
Comment at: migrate-tool/BuildManager.h:27
@@ +26,3 @@
+
+  virtual std::string getTargetForFile(llvm::StringRef File) = 0;
+};
----------------
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?

================
Comment at: migrate-tool/HeaderBuild.cpp:19
@@ +18,3 @@
+
+std::string joinStrings(const std::vector<std::string> &Strings) {
+    std::ostringstream SS;
----------------
Can't we use join from ADT/StringExtras.h?

================
Comment at: migrate-tool/HeaderBuild.h:19
@@ +18,3 @@
+// This constructs a C++ header containing type aliases.
+class Header {
+public:
----------------
I'd probably call this HeaderGenerator or something.


================
Comment at: migrate-tool/HeaderBuild.h:29
@@ +28,3 @@
+
+  std::string generateContent() const;
+
----------------
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?

================
Comment at: migrate-tool/RefactorManager.h:24
@@ +23,3 @@
+// performs some refactoring operations.
+class RefactorManager {
+public:
----------------
bkramer wrote:
> Documentation for the methods? :)
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.


https://reviews.llvm.org/D24380