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

[Devin Coughlin via Phabricator via cfe-commits]

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