[PATCH] D97738: [clangd] Move DraftStore from ClangdLSPServer into ClangdServer.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D97738#2597296 <https://reviews.llvm.org/D97738#2597296>, @kadircet wrote:

> as discussed offline, i am hesitant about `validateEdits` layering at the moment. but i suppose the best thing to do is move it into clangdserver and expose a structrual api, as you proposed, which can be done independently.

Yup. It's not great, but it doesn't seem terrible or dangerous or anything.  (We're not opening any new interfaces to support this, getDraft() is called in lots of places)
In the same bucket I think is sorting out TextEdits vs Replacements in a few interfaces etc.



================
Comment at: clang-tools-extra/clangd/ClangdServer.h:266
+  /// If \p Rng is non-null, formats only that region.
+  void formatFile(PathRef File, const Range *Rng,
                   Callback<tooling::Replacements> CB);
----------------
kadircet wrote:
> nit: Optional<Range> instead of a pointer?
Haha, I had this, then thought "but we pass things in by reference anyway".
Except... we don't, in ClangdServer. And I'm not sure why.

Anyway, done.


================
Comment at: clang-tools-extra/clangd/DraftStore.cpp:66
     // We treat versions as opaque, but the protocol says they increase.
-    if (*Version <= D.Version)
-      log("File version went from {0} to {1}", D.Version, Version);
-    D.Version = *Version;
+    if (SpecifiedVersion.compare_numeric(D.Version) <= 0)
+      log("File version went from {0} to {1}", D.Version, SpecifiedVersion);
----------------
kadircet wrote:
> why not a not_equals instead? we are going to override the version anyway
I don't really understand the suggestion.

The purpose here is to notice and log when client-specified versions go backwards.
This is a protocol violation, which won't cause problems for clangd but may indicate something broken going on.

We don't want to log when versions go forwards!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97738