[PATCH] D89135: [clangd] Stop capturing trace args if the tracer doesn't need them.

[Sam McCall via Phabricator via cfe-commits]

sammccall marked an inline comment as done.
sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/support/Trace.cpp:91
   public:
-    JSONSpan(JSONTracer *Tracer, llvm::StringRef Name, llvm::json::Object *Args)
+    llvm::json::Object Args;
+    JSONSpan(JSONTracer *Tracer, llvm::StringRef Name)
----------------
kadircet wrote:
> move this near other fileds?
It's public unlike the other fields, but moved it to the bottom of the public section


================
Comment at: clang-tools-extra/clangd/support/Trace.cpp:302
+                                            : llvm::StringRef(Name.str()),
+                   [&](llvm::json::Object *A) { Args = A; });
+  return std::make_pair(std::move(Ctx), Args);
----------------
added assertions here (nonnull and empty) to guard against any API confusion


================
Comment at: clang-tools-extra/clangd/support/Trace.h:86
+  beginSpan(llvm::StringRef Name,
+            llvm::function_ref<void(llvm::json::Object *)> RequestArgs);
   // Called when a Span is destroyed (it may still be active on other threads).
----------------
kadircet wrote:
> nit: at first i parsed request in `RequestArgs` as a verb (e.g. `i request arguments`), which was confusing a little bit. Hence the comment also didn't make much sense, I only managed to error correct after seeing the usage :D
> 
> Maybe something like `SetRequestArgs` to emphasize that one provides request arguments using this lambda rather than receiving them?
This *is* actually the intention.

Changed to `AttachDetails` to avoid ambiguity, and rewrote the comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89135