sammccall marked 3 inline comments as done.
sammccall added a comment.
In D77847#1974126 <https://reviews.llvm.org/D77847#1974126>, @kadircet wrote:
> should we also have a unittest for checking ast caching works as expected?
TUSchedulerTests has `NoopOnEmptyChanges` which I think tests exactly this, and
`EvictedAST` which checks it's sensitive to the cache.
I don't think testing it again indirectly at this level makes is a good
tradeoff, especially if we need subtle assertions via the memory usage.
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:575
+ {"change", (int)TextDocumentSyncKind::Incremental},
+ {"save", true},
+ }},
----------------
kadircet wrote:
> spec also specifies this as `property name (optional):
> textDocumentSync.didSave` near didSave notification (in addition to defining
> it as `save` in the struct). :(
>
> it also says textDocumentSync can also be a number for backward compat
> reasons, but doesn't say when the struct was added. I hope it is not recent
> and we don't end up breaking clients with our capab response :/
>
> no action needed just complaining ...
> spec also specifies this as property name (optional): textDocumentSync.didSave
Good catch. VSCode and every other impl I could find implement `save` though,
so I think that's just a spec bug. Sent
https://github.com/microsoft/language-server-protocol/pull/958
(We could set both here, but I don't think it's a good idea to muddy the water)
> it also says textDocumentSync can also be a number for backward compat
> reasons, but doesn't say when the struct was added. I hope it is not recent
> and we don't end up breaking clients with our capab response :/
yeah. I hadn't really internalized that all the nice compatibility hacks in the
spec are for servers, and don't help much with clients.
I think this is pretty old though, and there's not much we can do about it
anyway.
> no action needed just complaining ...
Ack, and agreed :-)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77847/new/
https://reviews.llvm.org/D77847
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
