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

[Sam McCall via Phabricator via llvm-commits]

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