[PATCH] D39050: Add index-while-building support to Clang

[Eric Liu via Phabricator via cfe-commits]

ioeric accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.

Nice! Thanks for making the refactoring and adding tests! I think this is good to go now.

I'm not very familiar with code outside of the index library (Driver, Basic etc), but the changes seem reasonable to me. Feel free to get another pair of eyes for them ;)



================
Comment at: include/clang/Index/RecordingAction.h:42
+std::unique_ptr<FrontendAction>
+createIndexDataRecordingAction(RecordingOptions RecordOpts,
+                               std::unique_ptr<FrontendAction> WrappedAction);
----------------
Add a FIXME that this is not implemented yet.


================
Comment at: lib/Index/IndexingAction.cpp:758
+
+  class IndexUnitDataRecorder : public IndexUnitDataConsumer {
+  public:
----------------
nathawes wrote:
> ioeric wrote:
> > I think the inheritance of `IndexUnitDataConsumer`  and the creation of factory should be in user code (e.g. implementation for on-disk persist-index-data should come from the compiler invocation code `ExecuteCompilerInvocation.cpp` or at least a separate file in the library that compiler invocation can use), and the user should only use `createUnitIndexingAction` by providing a factory.  Currently, `createUnitIndexingAction` and `createIndexDataRecordingAction` are mostly identical except for the code that implements `IndexUnitDataConsumer ` and creates the factory.
> > 
> > The current `createIndexDataRecordingAction` would probably only used by the compiler invocation, and we can keep the generalized `createUnitIndexingAction` in the public APIs.
> `IndexUnitDataRecorder` here is just a stub I added when I split the patch up – the follow-up revision has it in a separate file. I'll move the separate files to this patch and stub out the method bodies with TODOs instead.
> 
> I've made `createIndexDataRecordingAction` call `createUnitIndexingAction` to remove the duplication, and pulled it, `RecordingOptions` and `getRecordingOptionsFromFrontendOptions` to a new header (`RecordingAction.h`) that `ExecuteComilerInvocation.cpp` uses. Does that sound ok?
Sounds good. Thanks for the explanation! 


https://reviews.llvm.org/D39050