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