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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

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.



================
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);
----------------
nit: Optional<Range> instead of a pointer?


================
Comment at: clang-tools-extra/clangd/DraftStore.cpp:50
+      S.insert(I.base(), '1');
+      return;
+    }
----------------
nit: s/return/break/ same below


================
Comment at: clang-tools-extra/clangd/DraftStore.cpp:57
+    }
+    assert(*I == '9');
+    *I = '0'; // and keep incrementing to the left.
----------------
nit: i'd drop the assert


================
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);
----------------
why not a not_equals instead? we are going to override the version anyway


================
Comment at: clang-tools-extra/clangd/SourceCode.h:208
+llvm::Error applyChange(std::string &Contents,
+                        const TextDocumentContentChangeEvent &Change);
+
----------------
i was going to complain about sourcecode depending on protocol, but apparently it already does...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97738