[PATCH] D75603: [clangd] Add instrumentation mode in clangd for metrics collection.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/CodeComplete.h:141
+  /// Ideally this must be disabled when being used by ClangdLSPServer.
+  bool EnableInstrumentationMode = false;
+  std::function<void(const CodeCompleteResult&)>* RecordCCResults;
----------------
I'm wondering if we can simplify this interface a bit. I'm not sure why we need another callback rather than just returning the CodeCompleteResult in the usual way.

Another option:
 - if we invoke a callback for each completion instead of for the result set as a whole, we don't need to work out where to stash anything.
 - the work done after `addCandidate` is pretty trivial, so invoking a callback there provides basically all the information about the result set. The Top-N truncation is probably something you'd rather *not* have for analysis.
 - code completion always ends with the callback being invoked, so cross-result analysis can be done at that point.

So I think this could just be a single
`std::function<void(const CodeCompletion&, const SymbolQualitySignals &, const SymbolRelevanceSignals &)>`.
If it's non-null, addCandidate would call toCodeCompletion on the bundle and pass it to the callback at the end.


================
Comment at: clang-tools-extra/clangd/CodeComplete.h:218
+    // Signals captured when instrumentation is enabled during code completion.
+    std::shared_ptr<SymbolQualitySignals> QualitySignals;
+    std::shared_ptr<SymbolRelevanceSignals> RelevanceSignals;
----------------
why shared rather than unique?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75603/new/

https://reviews.llvm.org/D75603