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?
================
Comment at: clang/include/clang/CrossTU/CrossTranslationUnit.h:233
+ /// counter on successful load. Member `CanBegin` is used to signal, that the
+ /// import attempt should be made at the beginning. Member `WasSuccesful`
+ /// signifies whether the load is successfully finished. The counter is
----------------
martong wrote:
> Could you please add these descriptions above the member declarations?
Good point! Will do that.
================
Comment at: clang/include/clang/CrossTU/CrossTranslationUnit.h:245
+ unsigned &NumASTLoaded;
+ bool CanBegin;
+ bool WasSuccessful;
----------------
martong wrote:
> This name `CanBegin` sounds strange to me. Perhaps `Enabled`?
First I was thinking about making the variable names "read" as an english
sentence. However CanBegin does not appear during use, so it is fair that
Enable is a better name. Changing it...
================
Comment at: clang/include/clang/CrossTU/CrossTranslationUnit.h:251
+ /// it is responsible for handing out LoadPass instances.
+ class LoadGuard {
+ public:
----------------
martong wrote:
> I have a feeling that LoadPass and LoagGuard could be (should be) merged
> together.
LoadPass is the RAII object, the other is decider whether the threshold is
reached. Note also that LoadGuard owns the loaded-counter variable. IMHO these
should be two separate classes, and I think if they were to be implemented in
the header these two would be simple, and easy-to-understand classes.
================
Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:352
+
+CrossTranslationUnitContext::LoadPass::LoadPass(unsigned &NumASTLoaded,
+ bool CanBegin)
----------------
martong wrote:
> I think you could leave the implementation of LoadPass:: member functions in
> the header, because they are so small. And that way I think code
> comprehension could improve.
Good point!
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
