[PATCH] D64384: [WIP] Index-while-building

[Dmitri Gribenko via Phabricator via llvm-commits]

gribozavr added a comment.

Not a full review, just some comments that would help reduce the size of the patch, plus code organization concerns.

Could we avoid nested libraries and rename `clang/include/clang/Index/IndexWhileBuilding` to `clang/include/clang/IndexWhileBuilding` ?



================
Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:30
 #include "clang/Index/IndexDataConsumer.h"
-#include "clang/Index/IndexingAction.h"
 #include "clang/Lex/Lexer.h"
----------------
Please submit changes like these this `#include` removal separately to reduce the size of the patch.


================
Comment at: clang-tools-extra/clangd/FindSymbols.cpp:20
 #include "clang/Index/IndexSymbol.h"
-#include "clang/Index/IndexingAction.h"
 #include "llvm/Support/FormatVariadic.h"
----------------
Please submit changes like these this `#include` removal separately to reduce the size of the patch.


================
Comment at: clang-tools-extra/clangd/index/IndexAction.cpp:128
+      : WrapperFrontendAction(
+            llvm::make_unique<index::GenerateIndexAction>(C, Opts)),
         SymbolsCallback(SymbolsCallback), RefsCallback(RefsCallback),
----------------
Could you move index action into a separate header and inline `index::createIndexingAction` into its callers in a separate commit?


================
Comment at: clang/include/clang/Index/IndexAction.h:22
+
+/// This is primarily a hack - poking hole through FrontendAction encapsulation
+/// in order to access protected methods from IndexActionWrapper. FIXME?
----------------
+1. Instead of `WrapperFrontendAction`, could you implement a general frontend action multiplexer in `clang/include/clang/Frontend/FrontendAction.h` ? You can do that as a separate commit.


================
Comment at: clang/include/clang/Index/IndexDataConsumer.h:64
+    return true;
+  }
 
----------------
Changes to this file can be submitted as a separate patch.


================
Comment at: clang/include/clang/Index/IndexOptions.h:19
+
+struct IndexingOptions {
+  enum class SystemSymbolFilterKind {
----------------
Are these the declarations moved from `clang/include/clang/Index/IndexingAction.h`? I'd suggest to commit this header splitting separately to reduce the size of the patch.


================
Comment at: clang/include/clang/Index/IndexWhileBuilding/PathStorage.h:74
+#endif
\ No newline at end of file

----------------
Please add newlines (everywhere in the patch).




================
Comment at: clang/include/clang/Index/USRGeneration.h:100
 
-#endif // LLVM_CLANG_INDEX_USRGENERATION_H
-
+#endif
----------------
Unintended edit?


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

https://reviews.llvm.org/D64384