[PATCH] D89852: [clangd] Use SmallString instead of std::string in marshalling code; NFC

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

Sorry I don't really follow what we are gaining by changes from string to SmallString. Could you explain if I am missing something?

But I think it makes sense to get rid of Optionals in the Marshaller for `{Local,Remote}IndexRoot`, as there's no difference between `None` vs `empty` for them (I think).



================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:58
+    this->RemoteIndexRoot =
+        llvm::SmallString<256>(llvm::sys::path::convert_to_slash(
+            RemoteIndexRoot, llvm::sys::path::Style::windows));
----------------
we seem to be explicitly creating a smallstring out of std::string. and all of the usages of remoteindexroot seems to be just using a stringref. why are we doing this exactly? to make sure the addition of trailing separator below doesn't do reallocation ? If so this only happens once per each remote-index, right? Is it really worth all the hassle?

same for `LocalIndexRoot`.


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:379
     return URIString.takeError();
-  Location.FileURI = Strings.save(*URIString).begin();
+  Location.FileURI = Strings.save(URIString->str()).begin();
   Location.Start = fromProtobuf(Message.start());
----------------
don't we end up creating a string here anyways?

(same below)


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.h:100
   /// be missing at the same time.
-  llvm::Optional<std::string> RemoteIndexRoot;
-  llvm::Optional<std::string> LocalIndexRoot;
+  llvm::Optional<llvm::SmallString<256>> RemoteIndexRoot;
+  llvm::Optional<llvm::SmallString<256>> LocalIndexRoot;
----------------
what's the difference between `None` and empty string here ? Can we just get rid of Optional?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89852