sammccall added a comment.

This looks really useful! Main suggestion is to drop the added span and attach 
kind to the main span instead. (It's relevant to index too, not just to sema)



================
Comment at: clangd/CodeComplete.cpp:403
 
+StringRef printCompletionKind(enum CodeCompletionContext::Kind Kind) {
+  using CCKind = enum CodeCompletionContext::Kind;
----------------
hmm, this really looks like it belongs next to CodeCompletionContext::Kind?


================
Comment at: clangd/CodeComplete.cpp:503
 
+    trace::Span Span("Process sema results");
+    SPAN_ATTACH(Span, "total_sema_items", NumResults);
----------------
I think this span is going to be tiny (hard to see in some UIs!) and generally 
not add a lot of value itself.
The nicest thing would be to just grab the kind out of the CCcontext and attach 
it to the span in CodeCompleteFlow::run()

My instinct is that the #items can be dropped entirely - we already log the 
number of items from Sema considered (before Limit) so all we'd be losing is 
the number hidden/ineligible/destructor completions we drop here, which seems 
uninteresting for tracing purposes.
(But exposing that from the recorder is another option)


================
Comment at: clangd/CodeComplete.cpp:504
+    trace::Span Span("Process sema results");
+    SPAN_ATTACH(Span, "total_sema_items", NumResults);
+    SPAN_ATTACH(Span, "sema_completion_kind",
----------------
sema_results_prefilter?
the filtering is the only difference between this and sema_results logged below


================
Comment at: clangd/CodeComplete.cpp:918
 
+    SPAN_ATTACH(Tracer, "file", SemaCCInput.FileName);
     SPAN_ATTACH(Tracer, "sema_results", NSema);
----------------
FWIW, I have a (delayed, sorry) patch to add a span to all actions run by 
TUscheduler, which would log the filename. If that lands, *maybe* we don't want 
it in both places (it'd be the parent span of this one)? But not sure.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43377



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to