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