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

[Gabor Marton via Phabricator via llvm-commits]

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 implemented in this
+  /// section.
----------------
Perhaps this comment is not necessary, because it is not obvious where does the mentioned "section" ends and this is a bit redundant with the comments for the classes below.


================
Comment at: clang/include/clang/CrossTU/CrossTranslationUnit.h:229
+
+  /// The thresholding of AST-loads is implemented in this section.
+
----------------
We may not need this comment.


================
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
----------------
Could you please add these descriptions above the member declarations?


================
Comment at: clang/include/clang/CrossTU/CrossTranslationUnit.h:245
+    unsigned &NumASTLoaded;
+    bool CanBegin;
+    bool WasSuccessful;
----------------
This name `CanBegin` sounds strange to me. Perhaps `Enabled`?


================
Comment at: clang/include/clang/CrossTU/CrossTranslationUnit.h:251
+  /// it is responsible for handing out LoadPass instances.
+  class LoadGuard {
+  public:
----------------
I have a feeling that LoadPass and LoagGuard could be (should be) merged together.


================
Comment at: clang/lib/CrossTU/CrossTranslationUnit.cpp:352
+
+CrossTranslationUnitContext::LoadPass::LoadPass(unsigned &NumASTLoaded,
+                                                bool CanBegin)
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64753