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