Diff Detail
Event Timeline
Thanks for sharing this! I hope I will have time to dig into more detail.
At a high level, my main question is around layering.
Until now, lib/Index has mostly been a pretty abstract framework: it gathers information from ASTs and exposes it to callers, who then decide what to do with it.
This has served clangd pretty well: we've been able to hook into it, use our own storage, fix the occasional bug, and avoid a bunch of custom tree traversals.
My understanding is that historically XCode has used it in similar ways by out-of-tree consumers (possibly via libclang?)
This patch introduces another concrete consumer in-tree, which is great - making uses more visible helps everyone understand and change the API.
It's also well-layered! But much of it's in the Index library, which makes the layering more fragile:
- it's unclear to me whether strict layering is intended here - there seem to be a couple of violations (though this is preview code)
- it's not going to be clear to people arriving at the code which dependencies are OK, and whether IWB is special or one client of many
- as it stands, there's no technical enforcement of layering (so it may rot), nor a way to depend on the abstract part but not the concrete part. This makes changes to the concrete part scarier.
If the currently-abstract parts Index becomes coupled to IWB over time, it'll be a regression for how clangd is using the library. Maybe we can converge on the concrete consumers too, but carefully (and not necessarily!)
Can we move the concrete parts to IndexStore directories (both header and impls)? (EmitIndexAction, DeclOccurrenceGenerator, IndexDataFormat, IndexDataStore, IndexRecordWriter, PathStorage, half of IndexOptions, BitstreamUtil, FSUtil, FileIndexRecord)
clang/include/clang/Index/GenerateIndexAction.h | ||
---|---|---|
35 | (this seems like suspicious layering, but also seems to be unused) | |
clang/include/clang/Index/IndexOptions.h | ||
40 | This file mixes abstract indexing stuff (IndexingOptions) with concrete (RecordingOptions) | |
clang/lib/Index/GenerateIndexAction.cpp | ||
12 | I can't tell where this is used - it's suspicious if required |
Thanks for taking a look Sam!
I finished porting of all the changes on top of llvm.org master in the meantime. The updated patch contains all the code we'd like to land. Build (on macOS) and tests are fine.
I'll leave it as a single patch for now so we can finish discussion about layering and I'll split it into couple smaller patches once this is done.
I've put the actual index-while-building part into separate subdirectories for now. For now I went with IndexWhileBuilding name but I'm open to discussion.
clang/include/clang/Index/GenerateIndexAction.h | ||
---|---|---|
35 | Right. Removed. | |
clang/lib/Index/GenerateIndexAction.cpp | ||
12 | Removed. Thanks. |
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 ?
clang-tools-extra/clangd/ClangdUnit.cpp | ||
---|---|---|
30 | Please submit changes like these this #include removal separately to reduce the size of the patch. | |
clang-tools-extra/clangd/FindSymbols.cpp | ||
20 | Please submit changes like these this #include removal separately to reduce the size of the patch. | |
clang-tools-extra/clangd/index/IndexAction.cpp | ||
128 | Could you move index action into a separate header and inline index::createIndexingAction into its callers in a separate commit? | |
clang/include/clang/Index/IndexAction.h | ||
22 | +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. | |
clang/include/clang/Index/IndexDataConsumer.h | ||
64 | Changes to this file can be submitted as a separate patch. | |
clang/include/clang/Index/IndexOptions.h | ||
19 | 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. | |
clang/include/clang/Index/IndexWhileBuilding/PathStorage.h | ||
74 | Please add newlines (everywhere in the patch). | |
clang/include/clang/Index/USRGeneration.h | ||
100 | Unintended edit? |
clang/include/clang/Driver/Options.td | ||
---|---|---|
331 | Could you add a more detailed description? Even as a compiler engineer, I can't really tell what index-record-codegen-name really does or why I would want to turn it on. | |
clang/include/clang/Frontend/CompilerInstance.h | ||
180 | No need to write "\brief", it is the default. | |
180 | Please don't write what something is used for, write what something is. "A factory for wrappers around FrontendActions that compile imported modules." | |
810 | I don't see anything actually using GenModuleActionWrapper. | |
clang/lib/Frontend/CompilerInstance.cpp | ||
1110 | Please end comments with a period. |
clang/include/clang/Frontend/FrontendOptions.h | ||
---|---|---|
391 | Could we change "ignore system symbols" into a positive option ("record system symbols")? Throughout the patch please. | |
clang/include/clang/Index/CodegenNameGenerator.h | ||
46 | Why change to a raw pointer? We can use a unique_ptr to an incomplete type, as long as the destructor ~CodegenNameGenerator is defined in a file where the type is complete. | |
clang/lib/Index/CodegenNameGenerator.cpp | ||
31 | This code looks like a straight copy from clang/lib/AST/Mangle.cpp -- can it be de-duplicated? | |
clang/unittests/Index/IndexTests.cpp | ||
74 | This change can be a separate patch. | |
106 | Unclear why this test overrides these methods, if the implementation is identical to the base class. | |
112 | No need to add virtual when we already have override. |
clang/include/clang/Index/IndexAction.h | ||
---|---|---|
22 | Sent for review: https://reviews.llvm.org/D66125 |
Thanks for the review! I addressed just some minor details so far.
clang/include/clang/Frontend/CompilerInstance.h | ||
---|---|---|
810 | IIUC it's both used (lambda on CompilerInstance.cpp:L1136) and passed (CompilerInstance.cpp:L1112) to CompilerInstance of module dependencies in compileModuleImpl. | |
clang/unittests/Index/IndexTests.cpp | ||
106 | Removed these overrides. |
clang/lib/Frontend/CompilerInstance.cpp | ||
---|---|---|
1132 | My suggestion would be to change GenerateModuleAction::CreateASTConsumer to perform the indexing. Doing so will avoid the problem of having to compose FrontendActions (which we found in another patch to be tricky), and also will make the code a lot more straightforward, I think. WDYT? |
clang/lib/Index/CodegenNameGenerator.cpp | ||
---|---|---|
31 | Seems like you're right: https://reviews.llvm.org/D63535 |
clang/include/clang/Frontend/CompilerInstance.h | ||
---|---|---|
810 | I was wrong! I thought we are passing some data in the wrapper. Turned out we aren't and I was able to completely remove it. Thanks! |
clang/include/clang/Frontend/FrontendOptions.h | ||
---|---|---|
391 | Surprisingly it seems that the name is actually mostly determined by the fact that we want to index system symbols by default and that natural CLI for boolean argument with default positive value is optional flag with negative semantics. Specifically - the semantics/name of the flag OPT_index_ignore_system_symbols is pretty much given (or ?). I could rename just IndexIgnoreSystemSymbols and keep OPT_index_ignore_system_symbols but it feels less clear than current state. WDYT? |
- renamed "codegen name generator" -> "mangled name generator"
- ranemed flag index-record-codegen-name -> index-record-mangled-name
- removed wrapper over ASTNameGenerator from libIndex
I addressed the rest of the comments (excluding comments about splitting the patch).
Next step is to follow up on the discussion about FrontendActions/ASTConsumers from https://reviews.llvm.org/D66125 and then I'd like to start splitting off separate commits.
I'll keep this patch rebased on top of what has already landed to have some sense what still needs to be done.
I'm now taking a look at suggestions from https://reviews.llvm.org/D66125 regarding IndexingFrontendAction in libclang.
IndexingFrontendAction is a lot more complex -- not only it needs a complex factory that creates the ASTConsumer, but it also overrides getTranslationUnitKind, which changes the behavior of Sema and Preprocessor in significant ways. So it does not seem like IndexFrontendAction can become an ASTConsumer.
However, IndexFrontendAction does not seem like a standalone ASTConsumer anyway. Its primary reason to exist is the logic in IndexingConsumer::shouldSkipFunctionBody. That logic seems interesting for general-purpose indexing, not just for libclang, so it would make sense to fold it into IndexASTConsumer.
To summarize:
Fold shouldSkipBody from IndexingConsumer in libclang into IndexASTConsumer in libIndex.
Make libIndex expose an ASTConsumer factory function, that encapsulates the logic currently in IndexingFrontendAction::CreateASTConsumer. Clients who need to combine indexing with some other FrontendAction can use that factory to create a consumer that will index whatever comes out of Sema.
Make a FrontendAction that implements the getTranslationUnitKind function to implement the corresponding optimization. If this FrontendAction is only needed in libclang, it can live there, otherwise, it can be in libIndex.
What do you think?
IIUC the goal was to move as much of interesting logic to libIndex as possible and hopefully implement the rest of the IndexingFrontendAction as an ASTConsumer instead of FrontendAction.
Now, it seems to me:
- IndexingFrontendAction::getTranslationUnitKind() method just returns two different results based on configuration, there's no logic - we just pass the configuration via DataConsumer. I am not quite sure this is interesting enough to create a self-standing FrontendAction. (To be completely honest - this actually seems more like a hack but I am not familiar enough with how does TUKind affect Preprocessor and Sema to assess this.)
- We could factor out an ASTConsumer factory from IndexingFrontendAction::CreateASTConsumer but there are also preprocessor callbacks created, DataConsumer set up with CompilerInstance and CXIndexDataConsumer::importedPCH called. This means we'd probably end up with having a FrontendAction with CXIndexDataConsumer in libclang anyway as this doesn't seem like a code that would quite fit into the hypothetical ASTConsumer factory.
Overall - it doesn't seem to me we could make IndexingFrontendAction significantly simpler or could reuse much of its code elsewhere so I suggest we leave it as it is.
WDYT?
I'll start splitting off some self-standing parts that won't be affected by hypothetical changes at the FrontendAction/ASTConsumer level.
clang/include/clang/Index/IndexDataConsumer.h | ||
---|---|---|
64 | Done in cfd641d84a5 [clang][Index][NFC] Move IndexDataConsumer default implementation |
Rebased on TOT and removed noise from the patch as I've already landed couple refactoring patches.
Rebased on top of current master and also tentatively on top of SerializablePathCollection (previously PathStorage, PathIndexer) from https://reviews.llvm.org/D66854
Please submit changes like these this #include removal separately to reduce the size of the patch.