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

[Benjamin Kramer via cfe-commits]

bkramer added a subscriber: bkramer.
bkramer added a comment.

One round of llvm-API specifics.


================
Comment at: migrate-tool/BuildManager.h:22
@@ +21,3 @@
+public:
+  virtual bool addHeaderOnlyLibrary(llvm::StringRef HeaderPath) = 0;
+
----------------
These methods could use some documentation.

================
Comment at: migrate-tool/HeaderBuild.cpp:19
@@ +18,3 @@
+
+std::string joinStrings(const std::vector<std::string> &Strings) {
+    std::ostringstream SS;
----------------
There's llvm::join for this.

================
Comment at: migrate-tool/HeaderBuild.cpp:30
@@ +29,3 @@
+public:
+  Namespace(llvm::StringRef Name, bool IsTopLevel = false)
+      : Name(Name), IsTopLevel(IsTopLevel) {}
----------------
Is (Name != "" && IsTopLevel == true) a valid input for this? If not, add an assert.

================
Comment at: migrate-tool/HeaderBuild.cpp:34
@@ +33,3 @@
+  void addAlias(llvm::StringRef NewName, llvm::StringRef TypeName) {
+    std::ostringstream SS;
+    SS << "using " << NewName.str() << " = ::" << TypeName.str() << ";";
----------------
Use raw_string_ostream instead.

================
Comment at: migrate-tool/HeaderBuild.cpp:103
@@ +102,3 @@
+      CurNs = CurNs->addNestedNamespace(*I);
+    CurNs->addAlias(*NewNameSplitted.rbegin(), Entry.second);
+  }
----------------
NewNameSplitted.back()

================
Comment at: migrate-tool/MigrateTool.cpp:26
@@ +25,3 @@
+void MigrateTool::addFile(llvm::StringRef FilePath, llvm::StringRef Content) {
+  std::ofstream File(FilePath.str(), std::ofstream::out);
+  File << Content.str();
----------------
Use llvm::raw_fd_ostream.

================
Comment at: migrate-tool/MigrateTool.cpp:79
@@ +78,3 @@
+extractNamespaceFromQualifiedName(llvm::StringRef QualifiedName) {
+  auto Pos = QualifiedName.rfind(':');
+  return (Pos == llvm::StringRef::npos) ? llvm::StringRef()
----------------
`return QualifiedName.rsplit(':').second;`

================
Comment at: migrate-tool/MigrateTool.h:85
@@ +84,3 @@
+  // Create a new file with the given `Content`.
+  void addFile(llvm::StringRef FilePath, llvm::StringRef Content);
+
----------------
createFile? writeFile? `add` sounds like it adds something to the tool, which it doesn't.

================
Comment at: migrate-tool/RefactorManager.h:24
@@ +23,3 @@
+// performs some refactoring operations.
+class RefactorManager {
+public:
----------------
Documentation for the methods? :)

================
Comment at: migrate-tool/RefactorManager.h:31
@@ +30,3 @@
+  addIncludesToFiles(const std::set<llvm::StringRef> &Includes,
+                     const std::vector<std::string> &Files) = 0;
+
----------------
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.


https://reviews.llvm.org/D24380