[PATCH] D73916: [clang] Add `forceReload` clangd extension to 'textDocument/didChange'

[Sam McCall via Phabricator via cfe-commits]

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