dcoughlin added a comment.
Thanks Gabor! Some additional comments in line.
================
Comment at: include/clang/CrossTU/CrossTranslationUnit.h:118
+ ///
+ /// \return Returns a map with the loaded AST Units and the declarations
+ /// with the definitions.
----------------
Is this comment correct? Does it return a map?
================
Comment at: include/clang/CrossTU/CrossTranslationUnit.h:128
+ /// \brief This function merges a definition from a separate AST Unit into
+ /// the current one.
+ ///
----------------
I found this comment confusing. Is 'Unit' the separate ASTUnit or it the
current one?
================
Comment at: include/clang/CrossTU/CrossTranslationUnit.h:134
+
+ std::string getLookupName(const NamedDecl *ND);
+
----------------
If this is public API, it deserves a comment. If not, can we make it private?
================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:35
+namespace {
+// FIXME: This class is will be removed after the transition to llvm::Error.
+class IndexErrorCategory : public std::error_category {
----------------
Is this FIXME still relevant? What will need to be transitioned to llvm::Error
for it to be removed? Can you make the comment a bit more specific so that
future maintainers will know when/how to address the FIXME?
================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:194
+ handleAllErrors(IndexOrErr.takeError(), [&](const IndexError &IE) {
+ (bool)PropagatedErr;
+ PropagatedErr =
----------------
What is this cast for? Is it to suppress an analyzer dead store false positive?
I thought we got all those!
================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:199
+ case index_error_code::missing_index_file:
+ Context.getDiagnostics().Report(diag::err_fe_error_opening)
+ << ExternalFunctionMap
----------------
If I understand correctly, there is no way to call loadExternalAST() without
the potential for a diagnostic showing up to the user. Can this diagnostic
emission be moved to the client?
================
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:249
+CrossTranslationUnitContext::importDefinition(const FunctionDecl *FD,
+ ASTUnit *Unit) {
+ ASTImporter &Importer = getOrCreateASTImporter(Unit->getASTContext());
----------------
Does this method need to take the unit? Can it just get the ASTContext from
'FD'? If not, can we add an assertion that FD has the required relationship to
Unit? (And document it in the header doc.)
================
Comment at: tools/clang-func-mapping/ClangFnMapGen.cpp:56
+private:
+ std::string getLookupName(const FunctionDecl *FD);
+ void handleDecl(const Decl *D);
----------------
Is getLookupName() here used?
================
Comment at: tools/clang-func-mapping/ClangFnMapGen.cpp:72
+ SmallString<128> LookupName;
+ bool Res = index::generateUSRForDecl(D, LookupName);
+ assert(!Res);
----------------
Should this instead reuse the logic for generating a lookup name from
CrossTranslationUnitContext::getLookupName()? That way if we change how the
lookup name is generated we only have to do it in one place.
https://reviews.llvm.org/D34512
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
