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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
