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