[PATCH] D44272: [clangd] Support incremental document syncing

[Simon Marchi via Phabricator via cfe-commits]

simark marked 10 inline comments as done.
simark added inline comments.


================
Comment at: clangd/DraftStore.cpp:75
+      return llvm::make_error<llvm::StringError>(
+          llvm::formatv("Range's end position (line={0}, character={1}) is "
+                        "before start position (line={2}, character={3}).",
----------------
ilya-biryukov wrote:
> Could we add a format provide for `Position` to make this formatting code shorter?
> We'll need to define the corresponding specialization in `Protocol.h`:
> 
> ```
> namespace llvm {
>   template <>
>   struct format_provider<clang::clangd::Position> {
>     static void format(const clang::clangd::Position &Pos, raw_ostream &OS, StringRef Style) { 
>      assert(Style.empty() && "style modifiers for this type are not supported");
>      OS << Pos;
>     }
>   };
> }
> ```
> 
> This will allow to simplify this call site to:
> ```
> llvm::formatv("Range's end position ({0}) is before start position ({1})", End, Start)
> ```
Good idea, this is good for printing them in a consistent way too.


================
Comment at: clangd/DraftStore.cpp:100
+  EntryIt->second = Contents;
+  return std::move(Contents);
+}
----------------
ilya-biryukov wrote:
> Doing `return std::move(localvar)` is a pessimization. Use `return Contents;` instead.
>  (for reference see [[https://stackoverflow.com/questions/14856344/when-should-stdmove-be-used-on-a-function-return-value|this SO post]], clang should give a warning for that case too)
Thanks, good catch.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44272