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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
