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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
