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

[Nathan Hawes via Phabricator via cfe-commits]

nathawes added inline comments.


================
Comment at: lib/FrontendTool/ExecuteCompilerInvocation.cpp:170
+    Act = index::createIndexDataRecordingAction(FEOpts, std::move(Act));
+    CI.setGenModuleActionWrapper(&index::createIndexDataRecordingAction);
+  }
----------------
ioeric wrote:
> Could you comment on what this does? The `Act` above is already wrapped. Why do we need `setGenModuleActionWrapper` to `createIndexDataRecordingAction` again? Also, `createIndexDataRecordingAction` doesn't seem related to `GenModule`.
It's to wrap any GenerateModuleActions that get created as needed when/if Act ends up loading any modules, so that we output index data for them too. I'll add a comment.


================
Comment at: lib/Index/FileIndexRecord.cpp:39
+  auto It = std::upper_bound(Decls.begin(), Decls.end(), NewInfo);
+  Decls.insert(It, std::move(NewInfo));
+}
----------------
ioeric wrote:
> Why do we need `Decls` to be sorted by offset? If we want this for printing, it might make sense to just do a sort there.
It's mostly for when we hash them, so that ordering doesn't change the hash, but it's also for printing. The IndexASTConsumer doesn't always report symbol occurrences in source order, due to the preprocessor and a few other cases.
We can sort them when the IndexRecordDataConsumer's finish() is called rather than as they're added to avoid the copying from repeated insert calls if that's the concern.


================
Comment at: lib/Index/IndexingAction.cpp:504
+
+    CreatedASTConsumer = true;
+    std::vector<std::unique_ptr<ASTConsumer>> Consumers;
----------------
ioeric wrote:
>  Can we get this state from the base class instead of maintaining a another state, which seems to be identical?
I don't see this state in either base class (WrapperFrontendAction and IndexRecordActionBase). WrappingIndexAction and WrappingIndexRecordAction both have this, though. Were you thinking a new intermediate common base class between them and WrapperFrontendAction?


================
Comment at: lib/Index/IndexingAction.cpp:769
+  IndexCtx.setSysrootPath(SysrootPath);
+  Recorder.init(&IndexCtx, CI);
+
----------------
ioeric wrote:
> It's a bit worrying that `IndexDataRecorder` and `IndexContext` reference each other. If you only need some information from the `IndexingContext`, simply pass it into `Recorder`. In this case, I think you only need the `SourceManager`  from the `ASTContext` in the recorder to calculate whether a file is a system header. I see you also cache result of `IndexingContext::isSystemFile` in the indexing context, but I think it would be more sensible for the callers to handle caching for this call.
Good point. The IndexingContext was actually already calling IsSystemFile before it calls IndexDataRecorder's handleDeclOccurrence and handleModuleOccurrence anyway, so I'll change it to pass that through as an extra param and remove IndexDataRecorder's dependency on the IndexingContext.


https://reviews.llvm.org/D39050