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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
