balazske added a comment.
I discovered that there are problems with this change. The number of loaded AST
units is not incremented correctly and the "CTU loaded AST file" is displayed
for every access to a AST unit (even if it was got from cache). The problem is
that the counting and print of
This revision was automatically updated to reflect the committed changes.
Closed by commit rL367829: [CrossTU][NFCI] Refactor loadExternalAST function
(authored by gamesh411, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D64753?vs=213311=213315#toc
Repository:
rL LLVM
gamesh411 updated this revision to Diff 213311.
gamesh411 added a comment.
- Remove unused member Limit
- Rebase to current master
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64753/new/
https://reviews.llvm.org/D64753
Files:
martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.
LGTM! Thanks! Please do the commit.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64753/new/
https://reviews.llvm.org/D64753
gamesh411 updated this revision to Diff 212788.
gamesh411 added a comment.
Specify the exact meaning of successful storage
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64753/new/
https://reviews.llvm.org/D64753
Files:
martong added inline comments.
Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:489
+ if (DisplayCTUProgress) {
+if (llvm::Expected FileName =
+ASTStorage.getFileForFunction(LookupName, CrossTUDir, IndexName))
` LoadOperation` should not
gamesh411 marked 2 inline comments as done.
gamesh411 added a comment.
Updated the revision.
I find this solution definitely more compact with responsibilities more
separated. One more thing that comes to mind is that maybe the whole explicit
passing of CTUDir and IndexName arguments is a bit
gamesh411 updated this revision to Diff 212290.
gamesh411 marked 5 inline comments as done.
gamesh411 added a comment.
- Merge RAII class
- Update comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64753/new/
https://reviews.llvm.org/D64753
gamesh411 marked 11 inline comments as done.
gamesh411 added a comment.
Thanks for pointing out these issues. Most of them are agreed. Merging the RAII
counter with the threshold checker class, however, does not seem like a good
decision for me. What would be the benefits of merging the two?
martong added a comment.
I have a feeling that LoadPass and LoagGuard could be (should be) merged
together, other than that we are getting close.
Comment at: clang/include/clang/CrossTU/CrossTranslationUnit.h:185
+
+ /// Cached access to ASTUnit mapping information is
gamesh411 updated this revision to Diff 211363.
gamesh411 added a comment.
Too much autoformat fixed
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64753/new/
https://reviews.llvm.org/D64753
Files:
gamesh411 updated this revision to Diff 211361.
gamesh411 added a comment.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Refactor functionality into local classes
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64753/new/
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
gamesh411 created this revision.
gamesh411 added a reviewer: martong.
Herald added subscribers: cfe-commits, Szelethus, dkrupp, rnkovacs.
Herald added a project: clang.
Refactor loadExternalAST method of CrossTranslationUnitContext in order to
reduce maintenance burden and so that features are
14 matches
Mail list logo
