nathawes planned changes to this revision. nathawes marked 32 inline comments as done. nathawes added a comment.
@ioeric I should have an updated patch up shortly with your inline comments addressed + new tests. Thanks again for reviewing! ================ Comment at: include/clang/Index/IndexUnitDataConsumer.h:67 +private: + virtual void _anchor(); +}; ---------------- ioeric wrote: > Comment? Why do we actually need this? From [[ https://stackoverflow.com/questions/34913197/what-is-vtable-anchoring-and-how-does-it-work-in-a-shared-object | here ]], my understanding is that it's an optimization to avoid the vtable being included in multiple translation units. I'm not sure if that's actually a problem, I was just following IndexDataConsumer's lead. Added a comment. ================ Comment at: include/clang/Index/IndexingAction.h:114 +std::unique_ptr<FrontendAction> +createUnitIndexingAction(const UnitIndexingOptions &IndexOpts, + IndexUnitDataConsumerFactory ConsumerFactory, ---------------- ioeric wrote: > What is the intended user of this function? It's unclear how users could > obtain a `ConsumerFactory ` (i.e. `UnitDetails`) without the functionalities > in `UnitDataConsumerActionImpl `. > > (Also see comment in the implementation of `createIndexDataRecordingAction`.) Sorry, I'm not sure what you mean here. Users shouldn't need to know anything about `UnitDataConsumerActionImpl`, they just need to provide a lambda/function reference that takes a `CompilerInstance&` and a `UnitDetails` and returns an `IndexUnitDataConsumer` (or `UnitIndexDataConsumer` once I rename it). This gets called once per translation unit to get a distinct data consumer for each unit, i.e. for the main translation unit as well as for each of its dependent modules that the main unit's data consumer says should be indexed via `shouldIndexModuleDependency(...)`. ================ Comment at: include/clang/Index/IndexingAction.h:128 +RecordingOptions +getRecordingOptionsFromFrontendOptions(const FrontendOptions &FEOpts); + ---------------- ioeric wrote: > This is likely only useful for compiler invocation. I would put it in the > compiler invocation code. There's another public `index::` API for writing out index data for individual clang module files in the follow up patch that takes a `RecordingOptions` and is used externally, from Swift. This function's useful on the Swift side to get the `RecordingOptions` from `FrontendOptions` it has already set up. ================ Comment at: lib/Index/IndexingAction.cpp:137 + + CreatedASTConsumer = true; + std::vector<std::unique_ptr<ASTConsumer>> Consumers; ---------------- ioeric wrote: > nit: Move this after `Impl->createIndexASTConsumer(CI)`. > > Do we need to reset this flag? Calling `CreateASTConsumer ` multiple times on > the same instance seems to be allowed? Oops. Yes, we do :-) ================ Comment at: lib/Index/IndexingAction.cpp:504 + /// \returns true if \c Mod was indexed + static bool indexModule( + const CompilerInstance &CI, serialization::ModuleFile &Mod, ---------------- ioeric wrote: > Non-factory static method is often a code smell. Any reason not to make these > static methods private members? With that, you wouldn't need to pass along so > many parameters. You could make them `const` if you don't want members to be > modified. Sorry, there's missing context – they're used from another public API that's in the follow-up patch. I'll bring that over and make these top-level static functions, since they don't belong exclusively to IndexDataConsumerActionImpl. ================ Comment at: lib/Index/IndexingAction.cpp:511 + /// Get unit details for the given module file + static UnitDetails getUnitDetails(serialization::ModuleFile &Mod, + const CompilerInstance &CI, ---------------- ioeric wrote: > Why is this overload public while others are private? Aren't they all used > only in this class? Same as above – this is called from a public `index::` API in the follow-up patch. ================ Comment at: lib/Index/IndexingAction.cpp:758 + + class IndexUnitDataRecorder : public IndexUnitDataConsumer { + public: ---------------- 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? ================ Comment at: lib/Index/IndexingAction.cpp:765 + auto ConsumerFactory = + [&](const CompilerInstance &CI, UnitDetails UnitInfo) -> + std::unique_ptr<IndexUnitDataConsumer> { ---------------- ioeric wrote: > The `UnitInfo` is ignored? What do we actually need it for? It should be passed to IndexUnitDataRecorder to write out info about the unit itself. This was just me splitting the patch badly. https://reviews.llvm.org/D39050 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
