[PATCH] D64753: [CrossTU][NFCI] Refactor loadExternalAST function

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Thank you Endre, this patch is a great initiative.
However, I think we can do better encapsulation then just the reorganization of the functions:
We could encapsulate into a nested class `NameASTUnitMap` and the functions which operate on this (`getCachedASTUnitForName`,
`loadFromASTFileCached`).
We could do the same with `NameFileMap`, `lazyInitCTUIndex`,  `getASTFileNameForLookup`.
And we could also encapsulate `NumASTLoaded` and its related function (`checkThresholdReached`). In this case perhaps we could use RAII to increase the counter.



================
Comment at: clang/include/clang/CrossTU/CrossTranslationUnit.h:167
+  bool checkThresholdReached() const;
+  llvm::Error lazyInitCTUIndex(StringRef CrossTUDir, StringRef IndexName);
+  ASTUnit *getCachedASTUnitForName(StringRef LookupName) const;
----------------
Could you please provide comments for these new functions? E.g. it would be useful to know what is the `IndexName` param.


================
Comment at: clang/include/clang/CrossTU/CrossTranslationUnit.h:168
+  llvm::Error lazyInitCTUIndex(StringRef CrossTUDir, StringRef IndexName);
+  ASTUnit *getCachedASTUnitForName(StringRef LookupName) const;
+  std::unique_ptr<ASTUnit> loadFromASTFile(StringRef ASTFileName) const;
----------------
`LookupName` is the USR?


================
Comment at: clang/include/clang/CrossTU/CrossTranslationUnit.h:185
 
   llvm::StringMap<std::unique_ptr<clang::ASTUnit>> FileASTUnitMap;
   llvm::StringMap<clang::ASTUnit *> NameASTUnitMap;
----------------
Could you please add comments for these maps (caches) about what we use them for?


================
Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:364
+
+  if (auto IndexMapping = parseCrossTUIndex(IndexFile, CrossTUDir)) {
+    // Initialize member map.
----------------
I don't think we should use `auto` here, because it is not obvious what will be the return type. It is important to see that we deal with an `Expected<...>`.
Perhaps we should have a type alias for `llvm::StringMap<std::string>`.


================
Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:448
 
-    auto It = NameFileMap.find(LookupName);
-    if (It == NameFileMap.end()) {
-      ++NumNotInOtherTU;
-      return llvm::make_error<IndexError>(index_error_code::missing_definition);
-    }
-    StringRef ASTFileName = It->second;
-    auto ASTCacheEntry = FileASTUnitMap.find(ASTFileName);
-    if (ASTCacheEntry == FileASTUnitMap.end()) {
-      IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts = new DiagnosticOptions();
-      TextDiagnosticPrinter *DiagClient =
-          new TextDiagnosticPrinter(llvm::errs(), &*DiagOpts);
-      IntrusiveRefCntPtr<DiagnosticIDs> DiagID(new DiagnosticIDs());
-      IntrusiveRefCntPtr<DiagnosticsEngine> Diags(
-          new DiagnosticsEngine(DiagID, &*DiagOpts, DiagClient));
-
-      std::unique_ptr<ASTUnit> LoadedUnit(ASTUnit::LoadFromASTFile(
-          ASTFileName, CI.getPCHContainerOperations()->getRawReader(),
-          ASTUnit::LoadEverything, Diags, CI.getFileSystemOpts()));
-      Unit = LoadedUnit.get();
-      FileASTUnitMap[ASTFileName] = std::move(LoadedUnit);
-      ++NumASTLoaded;
-      if (DisplayCTUProgress) {
-        llvm::errs() << "CTU loaded AST file: "
-                     << ASTFileName << "\n";
-      }
-    } else {
-      Unit = ASTCacheEntry->second.get();
+  // Lazily initialize the mapping from function names to AST files.
+  if (llvm::Error InitFailed = lazyInitCTUIndex(CrossTUDir, IndexName))
----------------
This comment might be also placed in the .h file as a oneliner above the function's prototype.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64753/new/

https://reviews.llvm.org/D64753