This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE320576: [clangd] Implemented logging using Context
(authored by ibiryukov, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D40486?vs=126728&id=126729#toc
Repository:
rCTE Clang To
ilya-biryukov updated this revision to Diff 126728.
ilya-biryukov added a comment.
Merged with head
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40486
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnit.cpp
clangd/Clangd
ilya-biryukov updated this revision to Diff 126725.
ilya-biryukov marked 2 inline comments as done.
ilya-biryukov added a comment.
- Simplify ScopeExitGuard by using llvm::Optional<>.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40486
Files:
clangd/ClangdLSPServer.cpp
cla
sammccall accepted this revision.
sammccall added inline comments.
Comment at: clangd/Function.h:157
+
+ ScopeExitGuard(ScopeExitGuard &&Other)
+ : F(std::move(Other.F)), Moved(Other.Moved) {
ilya-biryukov wrote:
> sammccall wrote:
> > I'm struggling to thi
ilya-biryukov marked 13 inline comments as done.
ilya-biryukov added inline comments.
Comment at: clangd/ClangdServer.cpp:216
-std::future>
-ClangdServer::codeComplete(PathRef File, Position Pos,
+std::future, Context>>
+ClangdServer::codeComplete(Context Ctx, PathRef File, Pos
ilya-biryukov updated this revision to Diff 126712.
ilya-biryukov marked 13 inline comments as done.
ilya-biryukov added a comment.
- Removed obsolete comments.
- Moved ScopeExitGuard to namespace detail.
- Added FIXMEs to pass context to compilation database.
- Addded a FIXME to JSONOutput.
- Rem
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Thanks! Just style and comment nits left really.
Comment at: clangd/ClangdServer.cpp:216
-std::future>
-ClangdServer::codeComplete(PathRef File, Position Pos,
+std::f
ilya-biryukov updated this revision to Diff 126683.
ilya-biryukov added a comment.
These should be the last usages where Context is not a first paramter.
- Make Context the first parameter in findDefinitions and CppFile::rebuild.
- Make Context the first parameter in invokeCodeCompletion.
Repos
ilya-biryukov updated this revision to Diff 126680.
ilya-biryukov added a comment.
- Make Context the first parameter in rename and a few helper methods.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40486
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/
ilya-biryukov updated this revision to Diff 126554.
ilya-biryukov added a comment.
- Also check that return type of Callable is correct.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40486
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
ilya-biryukov added inline comments.
Comment at: clangd/Function.h:44
+/// A sfinae-check that Callable can be called with Args...
+class = decltype(std::declval()(std::declval()...),
+ void())>
We need this che
ilya-biryukov added a comment.
This is now ready for review.
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
ilya-biryukov updated this revision to Diff 126551.
ilya-biryukov added a comment.
- Make Context the first parameter everywhere.
- Add a SFINAE check to UniqueFunction's constructor to avoid creating it from
non-callable objects.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D
ilya-biryukov added a comment.
I think all the comments are addressed now and this change is ready for another
round of review.
Let me know if I missed something.
Comment at: clangd/ClangdServer.cpp:36
- ~FulfillPromiseGuard() { Promise.set_value(); }
+ ~FulfillContextProm
ilya-biryukov updated this revision to Diff 126544.
ilya-biryukov marked 7 inline comments as done.
ilya-biryukov added a comment.
- Added a helper that runs functions in destructor.
- Fixed compilation after rebase.
- Got rid of FulfilContextPromiseGuard
- Added a FIXME to remove the Ctx typedef.
sammccall added a comment.
In https://reviews.llvm.org/D40486#945193, @ilya-biryukov wrote:
> I'll update the implementation and place the `Context` as the first parameter
> everywhere instead.
> Could you take a look at other changes in the patch while I'm at it?
Just pinging this - there's
ilya-biryukov added inline comments.
Comment at: clangd/ClangdServer.cpp:36
- ~FulfillPromiseGuard() { Promise.set_value(); }
+ ~FulfillContextPromiseGuard() { Promise.set_value(std::move(Ctx)); }
sammccall wrote:
> Yikes, I can see how we got here, but we
ilya-biryukov updated this revision to Diff 126375.
ilya-biryukov added a comment.
- Use derive(key, val) instead of derive().add(key, val).
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40486
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.
ilya-biryukov updated this revision to Diff 126340.
ilya-biryukov added a comment.
- Remove mention of globalLogger() from the comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40486
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
ilya-biryukov added inline comments.
Comment at: clangd/ClangdServer.cpp:220
- // Note that std::future from this cleanup action is ignored.
- scheduleCancelRebuild(std::move(Recreated.RemovedFile));
+ // Note that std::future from this cleanup action.
+ // FIXME(ibiryukov)
ilya-biryukov updated this revision to Diff 126337.
ilya-biryukov marked 15 inline comments as done.
ilya-biryukov added a comment.
- Copy Context in forceReparse's call to scheduleCancelRebuild.
- Renamed Key<> for ID, Out and Span.
- Removed the FIXME
- Got rid of getExisting(RequestSpan)
- Remo
ilya-biryukov updated this revision to Diff 126327.
ilya-biryukov added a comment.
Udpated the patch after changes in Context
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40486
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/Clan
ilya-biryukov updated this revision to Diff 126316.
ilya-biryukov added a comment.
- Pass Context by const-ref instead of by-ref.
- Don't expose global logger, it is only used in log method now.
- Renamed logImpl to log.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40486
File
sammccall added inline comments.
Comment at: clangd/JSONRPCDispatcher.cpp:23
+namespace {
+static Key> TracerKey;
+static Key IDKey;
sammccall wrote:
> RequestTracer?
actually, RequestSpan I think - "tracer" is pretty confusing at global scope
sammccall added a comment.
Mostly nits - throughout there's going to be lots of judgement calls about
where to propagate context and where not.
No need to get all those "exactly right", but trying to get a feel for what
these answers are likely to be.
Most of the interesting stuff is around the
ilya-biryukov added a comment.
Additionally:
- All async methods now take `Context` by-value and pass it to their
`Callback`s or return them in `future`s.
- All sync methods take `Context` by-ref
In https://reviews.llvm.org/D40486#941210, @sammccall wrote:
> This is pretty bikesheddy, but I
ilya-biryukov updated this revision to Diff 125545.
ilya-biryukov added a comment.
Herald added a subscriber: klimek.
- Updated to match the new Context implementation.
- Logger is now stored in a global variable instead of the global Context.
- Removed json-specific RequestContext, now storing al
sammccall added a comment.
This is pretty bikesheddy, but I wonder what you think about passing Ctx as the
first vs last parameter.
First has some advantages (maybe I just read too much Go though):
- it's a short expr, and F(short, long) tends to format better than F(long,
short). Particularly
ilya-biryukov created this revision.
https://reviews.llvm.org/D40486
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/ClangdUnitStore.cpp
clangd/ClangdUnitStore.h
clangd/GlobalCompilationDatabase.cpp
29 matches
Mail list logo
