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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.


================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:82
+    size_t Pos = Line.find(" ");
+    StringRef LineRef{Line};
+    if (Pos > 0 && Pos != std::string::npos) {
----------------
danielmarjamaki wrote:
> LineRef can be const
StringRef is an immutable reference to a string, I think it is not idiomatic in LLVM codebase to make it const. 


================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:85
+      FunctionName = LineRef.substr(0, Pos);
+      if (Result.count(FunctionName))
+        return llvm::make_error<IndexError>(
----------------
danielmarjamaki wrote:
> I would like to see some FunctionName validation. For instance "123" should not be a valid function name.
This is not a real function name but an USR. I updated the name of the variable to reflect that this name is only used for lookup. 


================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:89
+      FileName = LineRef.substr(Pos + 1);
+      SmallString<256> FilePath = CrossTUDir;
+      llvm::sys::path::append(FilePath, FileName);
----------------
danielmarjamaki wrote:
> Stupid question .. how will this work if the path is longer than 256 bytes?
If the path is shorter than 256 bytes it will be stored on stack, otherwise `SmallString` will allocate on the heap.


================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:125
+CrossTranslationUnitContext::findFunctionInDeclContext(const DeclContext *DC,
+                                                       StringRef LookupFnName) {
+  assert(DC && "Declaration Context must not be null");
----------------
danielmarjamaki wrote:
> LookupFnName could be const right?
In LLVM we usually do not mark StringRefs as consts, they represent a const reference to a string.


================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:223
+    assert(ToDecl->hasBody());
+    assert(FD->hasBody() && "Functions already imported should have body.");
+    return ToDecl;
----------------
danielmarjamaki wrote:
> 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?
Yes, after the importer imports the new declaration with a body we should be able to find the body through the original declaration. The importer modifies the AST and the supporting data structures. 


https://reviews.llvm.org/D34512