[PATCH] D40486: [clangd] Implemented logging using Context

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked 13 inline comments as done.
ilya-biryukov added inline comments.


================
Comment at: clangd/ClangdServer.cpp:216
 
-std::future<Tagged<CompletionList>>
-ClangdServer::codeComplete(PathRef File, Position Pos,
+std::future<std::pair<Tagged<CompletionList>, Context>>
+ClangdServer::codeComplete(Context Ctx, PathRef File, Position Pos,
----------------
sammccall wrote:
> this is a pretty gross return type.
> Even after (if?) we replace tagged with context, we'll still have completionlist and context.
> Should we define a struct here?
> 
> (If not, we should consider having the context first, for consistency)
I agree, callback-based APIs look way better though.
Since we'll eventually switch to callback-based versions anyway, I'm gonna leave it as `pair` for now.


================
Comment at: clangd/ClangdServer.cpp:558
       [this, FileStr, Version,
-       Tag](UniqueFunction<llvm::Optional<std::vector<DiagWithFixIts>>()>
+       Tag](UniqueFunction<llvm::Optional<std::vector<DiagWithFixIts>>(
+                const Context &)>
----------------
sammccall wrote:
> This wrapping has gone beyond the point of readability for me.
> Pull out using `RebuildAction = llvm::Optional<...>(const Context &)`?
I'd refrain from doing that as this code will go away in the threading patch anyway. Hope that's ok.


================
Comment at: clangd/ClangdUnit.cpp:555
+                std::shared_ptr<PCHContainerOperations> PCHs) {
+  return std::shared_ptr<CppFile>(new CppFile(
+      FileName, std::move(Command), StorePreamblesInMemory, std::move(PCHs)));
----------------
sammccall wrote:
> (while here, this should really be `make_shared`)
It can't, because `CppFile`'s constructor is private. This code is also going away in the threading patch.


================
Comment at: clangd/Function.h:143
+template <class Func> struct ScopeExitGuard {
+  static_assert(std::is_same<typename std::decay<Func>::type, Func>::value,
+                "Func must be decayed");
----------------
sammccall wrote:
> FWIW I think this assert adds more noise than value - this can clearly only be triggered by someone instantiating the template directly, which is probably better avoided using `namespace detail` or so.
Moved it to `namespace detail`.


================
Comment at: clangd/Function.h:157
+
+  ScopeExitGuard(ScopeExitGuard &&Other)
+      : F(std::move(Other.F)), Moved(Other.Moved) {
----------------
sammccall wrote:
> I'm struggling to think of cases where moving these guards is the right solution.
> Can we delete the move ops until we need them? It seems to make this class almost trivial.
We need moves as we return the class by value from the `onScopeGuard` function. Neither of these will compile without move constructors:
```
auto Guard = onScopeGuard([]() { /*...*/ });
auto Guard2(onScopeGuard([]() { /*...*/ }));
auto Guard3{onScopeGuard([]() { /*...*/ })};
```



================
Comment at: clangd/JSONRPCDispatcher.cpp:145
+
+  auto Ctx = Context::empty()
+                 .derive(RequestOut, &Out)
----------------
sammccall wrote:
> There are *lots* of `Context::empty()` calls in this file.
> Maybe conceptually clearer for JSONRPCDispatcher to own a "background" context that it derives from? It'd be empty for now in either case, so not a big deal.
> 
> The idea being that explicit Context::empty() calls are a bit suspect, but these ones aren't really "different" from each other - we may want to provide an extension point to customize it, but there'd only be one.
> 
> Up to you.
Since these usages are easy to spot (almost all uses of `Context::empty` in JSONRPCDispatcher.cpp), I'd leave them as is for now. It's easy to update them later.


================
Comment at: clangd/JSONRPCDispatcher.cpp:148
+                 .derive(RequestSpan, std::move(Tracer));
+  if (ID)
+    Ctx = std::move(Ctx).derive(RequestID, *ID);
----------------
sammccall wrote:
> I'm not sure this is better than unconditionally attaching the optional `ID`.
> Being able to distinguish between request-with-no-ID and no-request seems like a good thing.
I like that we can avoid having a two-level unwrap for `RequestID`. If we're going down that route, maybe we should have a dedicated struct instead:
```
// name may be bad, just for the sake of example.
struct RequestData { 
  llvm::Optional<json::Expr> RequestID;
  Span RequestSpan;
  JSONOutput Out;
};

Key<RequestData> ReqData;
```

That would also make it clear that we either attach all of those fields or none of them.
Given that `RequestID` is not yet public, I'd keep it as is and change it in a separate patch.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40486