[PATCH] D83822: [clangd] Support config over LSP.

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 2 inline comments as done.
sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1220
       CDB->setCompileCommand(File, std::move(New));
       ModifiedFiles.insert(File);
     }
----------------
kadircet wrote:
> nit: maybe just set `ReparseAllFiles` in here too, and change the condition below to only `return ReparseAllFiles`
Is the idea here that triggering spurious reparses is cheap enough?
Or that if it's cheap enough for our "future-facing" extension, we should be able to stop optimizing it for the existing one?


================
Comment at: clang-tools-extra/clangd/Protocol.h:494
+  // Each value is an Object or null, and replaces any fragment with that key.
+  // Fragments are used in key order ("!foo" is low-priority, "~foo" is high).
+  std::map<std::string, llvm::json::Value> fragments;
----------------
kadircet wrote:
> it feels like clients will only be using two types of keys, a "global" one for sending over (likely project-specific) user preferences and another for directory/file specific configs. The latter is likely to have the path as a key, whereas the former will likely be a special identifier. I am glad that `!` comes before both `/` and any capital letters(letter drives).
> 
> I think it is only important to have a distinction between these two types (a subset of what you describe here). Current design makes it really easy on our side, I am just afraid of confusing clients. Maybe we should just have an enum/boolean tag saying if the fragment is global or not instead of relying on the key orderings ?
If we go down this path, where do we draw the line?
(this isn't a slipper-slope argument - I think it's a reasonable idea and we should work out how far we want to take it)

If we're encoding this file/directory use case in the protocol somehow, why wouldn't we *require* the key to be a path, and no longer require a condition to be set for that? This is like how `.clangd` files are handled.

Then we'd have a more natural way of dealing with priority between file/dir-specific entries (actually, maybe it coincides with sort order, ha!).

But this model starts to look something much closer to the `workspace/configuration` server->client request, which has a scope URI attached. The recommendation from LSP folks is that actual configuration flow over that endpoint, and didChangeConfiguration just be used to notify of changes.

That hasn't seemed feasible for us as we imagine all the requests issues by background index, and the bother of making everything consuming config asynchronous. But:
 - if we restricted per-URI config to per-directory, not per-file, the number of requests aren't overwhelming (assuming caching). It's still possible to achieve per-file config with conditions...
 - async seems fundamentally solvable, given that we only create config on TUScheduler threads and background-index threads now, never the main thread (unless in sync mode...). Technical details to work out of course (if the server doesn't get a response it'll want to time out, but what thread does the timeout handler run on?)

It would help there were some clearer practical goal we were driving towards. Propagating, say, vscode extension settings live into clangd can be done this way, but it's a sledgehammer for a peanut. Maybe live-configuring *workspace* properties is an interesting thing, but I'm not sure how much value there is when `.clangd` exists.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83822/new/

https://reviews.llvm.org/D83822