[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

small nits



================
Comment at: include/clang/CrossTU/CrossTranslationUnit.h:58
+
+/// \brief This function can parse an index file that determines which
+///        translation unit contains which definition.
----------------
I suggest that "can" is removed.


================
Comment at: include/clang/CrossTU/CrossTranslationUnit.h:62
+/// The index file format is the following:
+/// each line consists of an USR separated by a filepath.
+llvm::Expected<llvm::StringMap<std::string>>
----------------
there is no \return or \returns here.


================
Comment at: include/clang/CrossTU/CrossTranslationUnit.h:62
+/// The index file format is the following:
+/// each line consists of an USR separated by a filepath.
+llvm::Expected<llvm::StringMap<std::string>>
----------------
danielmarjamaki wrote:
> there is no \return or \returns here.
maybe: each line consists of an USR and a filepath separated by a space


================
Comment at: include/clang/CrossTU/CrossTranslationUnit.h:68
+
+/// \brief This class can be used for tools that requires cross translation
+///        unit capability.
----------------
Maybe /can be/is/ unless you envision that tools that require cross translation unit capability might use some other classes.


================
Comment at: include/clang/CrossTU/CrossTranslationUnit.h:92
+  /// definition of the function will be merged into the original AST using
+  /// the AST Importer. The declaration with the definition will be returned.
+  /// If no suitable definition is found in the index file, null will be
----------------
you should split out some of this information to a \return or \returns


================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:60
+
+char IndexError::ID = 0;
+
----------------
redundant assignment


================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:79
+  std::string Line;
+  unsigned LineNo = 0;
+  while (std::getline(ExternalFnMapFile, Line)) {
----------------
I suggest that LineNo is 1 on the first line.


================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:81
+  while (std::getline(ExternalFnMapFile, Line)) {
+    size_t Pos = Line.find(" ");
+    StringRef LineRef{Line};
----------------
Pos can be const


================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:82
+    size_t Pos = Line.find(" ");
+    StringRef LineRef{Line};
+    if (Pos > 0 && Pos != std::string::npos) {
----------------
LineRef can be const


================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:84
+    if (Pos > 0 && Pos != std::string::npos) {
+      FunctionName = LineRef.substr(0, Pos);
+      if (Result.count(FunctionName))
----------------
FunctionName and FileName can be moved here and it is possible to make these variables const.


================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:85
+      FunctionName = LineRef.substr(0, Pos);
+      if (Result.count(FunctionName))
+        return llvm::make_error<IndexError>(
----------------
I would like to see some FunctionName validation. For instance "123" should not be a valid function name.


================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:89
+      FileName = LineRef.substr(Pos + 1);
+      SmallString<256> FilePath = CrossTUDir;
+      llvm::sys::path::append(FilePath, FileName);
----------------
Stupid question .. how will this work if the path is longer than 256 bytes?


================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:102
+createCrossTUIndexString(const llvm::StringMap<std::string> &Index) {
+  std::stringstream Result;
+  for (const auto &E : Index) {
----------------
how about std::ostringstream , imho that is cleaner


================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:121
+
+/// Recursively visits the funtion decls of a DeclContext, and looks up a
+/// function based on USRs.
----------------
/funtion/function/


================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:125
+CrossTranslationUnitContext::findFunctionInDeclContext(const DeclContext *DC,
+                                                       StringRef LookupFnName) {
+  assert(DC && "Declaration Context must not be null");
----------------
LookupFnName could be const right?


================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:148
+
+  std::string LookupFnName = getLookupName(FD);
+  if (LookupFnName.empty())
----------------
I believe LookupFnName can be const


================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:189
+      return nullptr; // No definition found even in some other build unit.
+    ASTFileName = It->second;
+    auto ASTCacheEntry = FileASTUnitMap.find(ASTFileName);
----------------
It is possible to make ASTFileName const


================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:223
+    assert(ToDecl->hasBody());
+    assert(FD->hasBody() && "Functions already imported should have body.");
+    return ToDecl;
----------------
sorry I am probably missing something here.. you first assert !FD->hasBody() and here you assert FD->hasBody(). Is it changed in this function somewhere?


================
Comment at: tools/clang-func-mapping/ClangFnMapGen.cpp:78
+              SM.getFileEntryForID(SM.getMainFileID())->getName();
+          char *Path = realpath(SMgrName.str().c_str(), nullptr);
+          CurrentFileName = Path;
----------------
I believe that realpath() is a posix function


================
Comment at: unittests/CrossTU/CrossTranslationUnitTest.cpp:40
+    }
+    assert(FD);
+    bool OrigFDHasBody = FD->hasBody();
----------------
should this be assert(FD && FD->getName() == "f")


https://reviews.llvm.org/D34512