sammccall added a comment. Thanks for this. It makes me a bit sad but I don't really see a clean way to make this "just work".
================ Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:653 - Server->addDocument(File, *Contents, WantDiags); + Server->addDocument(File, *Contents, WantDiags, ForceReload); } ---------------- Rather than plumbing this down many levels as yet-another-bool-param, can we shove it into either ParseInputs or ParseOptions? e.g. `bool ParseInputs::AllowCached = true;` and set it to false here? It's kind of a stretch for the API as we won't make use of it everywhere that ParseInputs is used, but I think it's better than adding a niche boolean param everywhere. ================ Comment at: clang-tools-extra/clangd/Protocol.h:656 + + /// Force the preamble to be rebuilt for this version of the file (only when + /// present and set to true). This is a workaround for how our heuristics for ---------------- Nit: the preamble is an implementation detail here, an even higher-level description would be e.g. Force a complete rebuild of the file, ignoring all cached state. Slow! This is useful to defeat clangd's assumption that missing headers stay missing. ================ Comment at: clang-tools-extra/clangd/Protocol.h:661 + /// This is a clangd extension. + llvm::Optional<bool> forceReload; }; ---------------- just `bool forceReload = false;` - we don't bother modelling this as a tristate if there's a well-defined defaut. (Though I'm sure there are places where we are inconsistent...) ================ Comment at: clang-tools-extra/clangd/Protocol.h:661 + /// This is a clangd extension. + llvm::Optional<bool> forceReload; }; ---------------- sammccall wrote: > just `bool forceReload = false;` - we don't bother modelling this as a > tristate if there's a well-defined defaut. > > (Though I'm sure there are places where we are inconsistent...) somehow "rebuild" seems clearer than "reload" to me, because we're not being very specific about what we're loading. Also `allowCache` (with inverted sense) might be clearer. ================ Comment at: clang-tools-extra/clangd/TUScheduler.cpp:435 std::shared_ptr<const PreambleData> OldPreamble = getPossiblyStalePreamble(); ---------------- no need to pass a separate parameter through to buildPreamble - if you're forcing a preamble rebuild then just don't bother getting the old preamble. OldPreamble is null and you get the behaviour you want. (The logging will be slightly off - "first preamble" - but I don't think it's worth fixing) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73916/new/ https://reviews.llvm.org/D73916 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits