[PATCH] D77847: [clangd] Rebuild dependent files when a header is saved.

[Sam McCall via Phabricator via cfe-commits]

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