[PATCH] D64384: [WIP] Index-while-building
Sam McCall via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jul 9 14:31:22 PDT 2019
sammccall added a comment.
Herald added a subscriber: wuzish.
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)
================
Comment at: clang/include/clang/Index/GenerateIndexAction.h:34
+ class IndexDataConsumer;
+ class IndexUnitWriter;
+
----------------
(this seems like suspicious layering, but also seems to be unused)
================
Comment at: clang/include/clang/Index/IndexOptions.h:39
+
+struct RecordingOptions {
+ enum class IncludesRecordingKind {
----------------
This file mixes abstract indexing stuff (IndexingOptions) with concrete (RecordingOptions)
================
Comment at: clang/lib/Index/GenerateIndexAction.cpp:11
+
+#include "clang/Index/IndexDataFormat.h"
+#include "IndexPPCallbacks.h"
----------------
I can't tell where this is used - it's suspicious if required
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64384/new/
https://reviews.llvm.org/D64384
More information about the llvm-commits
mailing list