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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1220
       CDB->setCompileCommand(File, std::move(New));
       ModifiedFiles.insert(File);
     }
----------------
sammccall wrote:
> 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?
none :( sorry i misread this and thought the code below was doing `ModifiedFiles.size() != 0` (that's way the comment was a nit)

OTOH, as you noted spurious reparses are cheap(still some IO and a task in the queue). We expect the usage of updating compile commands to migrate towards configs in the future, so in theory we won't be using that optimization most of the time.

Even though we've got enough info to prevent spurious reparses here, we might not in the future (e.g. reparses resulting from a config change). So I would drop the optimization(the whole Old-New comparison logic) to increase readability.

but definitely not something we should do now (probably ever), so feel free to ignore.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83822