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

[Endre Fülöp via Phabricator via cfe-commits]

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