[clangd] Get rid of llvm::Optional in Remote- and LocalIndexRoot; NFC

[njames93]

I'm a little concerned by this, not sure if Expected and Optional play nicely with rvo. If the compiler can't optimise it's potentially a 256 byte memcpy in the return, granted that would still be cheaper than dynamic allocation. However it would most likely be faster if the functions took a reference to the SmallString (or more likely SmallVectorImpl<char>) and returned an llvm::Error or bool.

[kadircet]

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).

clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp

59

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.

385

don't we end up creating a string here anyways?

(same below)

clang-tools-extra/clangd/index/remote/marshalling/Marshalling.h

99–100

what's the difference between None and empty string here ? Can we just get rid of Optional?

[kbobyrev]

Get rid of llvm::Optional in local and remote paths.

[kadircet]

clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp

58–59

nit: if(!is_separator(this->RemoteIndexRoot.back(), posix)) same below.

[kbobyrev]

Resolve post-LGTM comment.

[kbobyrev]

Rebase on top of master

[ArcsinX]

clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp

60

I know this already commited, but maybe we could use llvm::sys::path::is_separator instead of is_separator to be consistent with other function calls from llvm::sys::path namespace in this file.

P.S. llvm::sys::path::is_separator checks a single char, but llvm::sys::path::get_separator returns string. Can not understand why. Could there be a multi-char separator in the future?

[kbobyrev]

clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp

60

I think it'd be better to strip llvm::sys::path elsewhere and simply rely on ADL. Spelling llvm::sys::path:: twice does not really add much value and simply takes more space. I'll send a separate patch.

[kadircet]

clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp

60

Because llvm::sys::path::get_separator returns a null terminated string.

Could there be a multi-char separator in the future?

I would expect the person doing such a change to also update is_separator.

[ArcsinX]

clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp

60

Because llvm::sys::path::get_separator returns a null terminated string.

Why does not llvm::sys::path::get_separator() return a single char instead of a null-terminated string?

[kadircet]

clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp

60

Why does not llvm::sys::path::get_separator() return a single char instead of a null-terminated string?

Probably to make composition with + easier. But one would need to dig-up the git blame and see if there are any other reasons I suppose, or ask the authors as mine is merely a guess :D

clang-tools-extra/clangd/index/remote/marshalling/Marshalling.h

private:
   llvm::Expected<clangd::Symbol::IncludeHeaderWithReferences>
   fromProtobuf(const v1::HeaderWithReferences &Message);
 
   /// RemoteIndexRoot and LocalIndexRoot are absolute paths to the project (on
   /// remote and local machine respectively) and include a trailing slash. One
   /// of them can be missing (if the machines are different they don't know each
   /// other's specifics and will only do one-way translation), but both can not
   /// be missing at the same time.
-  llvm::Optional<std::string> RemoteIndexRoot;
-  llvm::Optional<std::string> LocalIndexRoot;
+  std::string RemoteIndexRoot;
+  std::string LocalIndexRoot;
   llvm::BumpPtrAllocator Arena;
   llvm::UniqueStringSaver Strings;
 };
 
 } // namespace remote
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_REMOTE_MARSHALLING_H

clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp

 Marshaller::Marshaller(llvm::StringRef RemoteIndexRoot,
                        llvm::StringRef LocalIndexRoot)
     : Strings(Arena) {
   llvm::StringRef PosixSeparator =
       llvm::sys::path::get_separator(llvm::sys::path::Style::posix);
   if (!RemoteIndexRoot.empty()) {
     assert(llvm::sys::path::is_absolute(RemoteIndexRoot));
     this->RemoteIndexRoot = llvm::sys::path::convert_to_slash(
         RemoteIndexRoot, llvm::sys::path::Style::windows);
-    llvm::StringRef Path(*this->RemoteIndexRoot);
-    if (!Path.endswith(PosixSeparator))
-      *this->RemoteIndexRoot += PosixSeparator;
+    llvm::StringRef Path(this->RemoteIndexRoot);
+    if (!is_separator(this->RemoteIndexRoot.back(),
+                      llvm::sys::path::Style::posix))
+      this->RemoteIndexRoot += PosixSeparator;
   }
   if (!LocalIndexRoot.empty()) {
     assert(llvm::sys::path::is_absolute(LocalIndexRoot));
     this->LocalIndexRoot = llvm::sys::path::convert_to_slash(
         LocalIndexRoot, llvm::sys::path::Style::windows);
-    llvm::StringRef Path(*this->LocalIndexRoot);
-    if (!Path.endswith(PosixSeparator))
-      *this->LocalIndexRoot += PosixSeparator;
+    llvm::StringRef Path(this->LocalIndexRoot);
+    if (!is_separator(this->LocalIndexRoot.back(),
+                      llvm::sys::path::Style::posix))
+      this->LocalIndexRoot += PosixSeparator;
   }
   assert(!RemoteIndexRoot.empty() || !LocalIndexRoot.empty());
 }
 
 llvm::Expected<clangd::LookupRequest>
 Marshaller::fromProtobuf(const v1::LookupRequest *Message) {
   clangd::LookupRequest Req;
   auto IDs = getIDs(Message->ids());
   if (!IDs)
     return IDs.takeError();
   Req.IDs = std::move(*IDs);
   return Req;
 }
 
 llvm::Expected<clangd::FuzzyFindRequest>
 Marshaller::fromProtobuf(const v1::FuzzyFindRequest *Message) {
-  assert(RemoteIndexRoot);
+  assert(!RemoteIndexRoot.empty());
   clangd::FuzzyFindRequest Result;
   Result.Query = Message->query();
   for (const auto &Scope : Message->scopes())
     Result.Scopes.push_back(Scope);
   Result.AnyScope = Message->any_scope();
   if (Message->limit())
     Result.Limit = Message->limit();
   Result.RestrictForCodeCompletion = Message->restricted_for_code_completion();
   for (const auto &Path : Message->proximity_paths()) {
-    llvm::SmallString<256> LocalPath = llvm::StringRef(*RemoteIndexRoot);
+    llvm::SmallString<256> LocalPath = llvm::StringRef(RemoteIndexRoot);
     llvm::sys::path::append(LocalPath, Path);
     // FuzzyFindRequest requires proximity paths to have platform-native format
     // in order for SymbolIndex to process the query correctly.
     llvm::sys::path::native(LocalPath);
     Result.ProximityPaths.push_back(std::string(LocalPath));
   }
   for (const auto &Type : Message->preferred_types())
     Result.ProximityPaths.push_back(Type);
v1::LookupRequest Marshaller::toProtobuf(const clangd::LookupRequest &From) {
   v1::LookupRequest RPCRequest;
   for (const auto &SymbolID : From.IDs)
     RPCRequest.add_ids(SymbolID.str());
   return RPCRequest;
 }
 
 v1::FuzzyFindRequest
 Marshaller::toProtobuf(const clangd::FuzzyFindRequest &From) {
-  assert(LocalIndexRoot);
+  assert(!LocalIndexRoot.empty());
   v1::FuzzyFindRequest RPCRequest;
   RPCRequest.set_query(From.Query);
   for (const auto &Scope : From.Scopes)
     RPCRequest.add_scopes(Scope);
   RPCRequest.set_any_scope(From.AnyScope);
   if (From.Limit)
     RPCRequest.set_limit(*From.Limit);
   RPCRequest.set_restricted_for_code_completion(From.RestrictForCodeCompletion);
   for (const auto &Path : From.ProximityPaths) {
     llvm::SmallString<256> RelativePath = llvm::StringRef(Path);
-    if (llvm::sys::path::replace_path_prefix(RelativePath, *LocalIndexRoot, ""))
+    if (llvm::sys::path::replace_path_prefix(RelativePath, LocalIndexRoot, ""))
       RPCRequest.add_proximity_paths(llvm::sys::path::convert_to_slash(
           RelativePath, llvm::sys::path::Style::windows));
   }
   for (const auto &Type : From.PreferredTypes)
     RPCRequest.add_preferred_types(Type);
   return RPCRequest;
 }
 
Marshaller::toProtobuf(const clangd::SymbolID &Subject,
   if (!SerializedObject)
     return SerializedObject.takeError();
   *Result.mutable_object() = *SerializedObject;
   return Result;
 }
 
 llvm::Expected<std::string>
 Marshaller::relativePathToURI(llvm::StringRef RelativePath) {
-  assert(LocalIndexRoot);
+  assert(!LocalIndexRoot.empty());
   assert(RelativePath == llvm::sys::path::convert_to_slash(RelativePath));
   if (RelativePath.empty())
     return error("Empty relative path.");
   if (llvm::sys::path::is_absolute(RelativePath, llvm::sys::path::Style::posix))
     return error("RelativePath '{0}' is absolute.", RelativePath);
-  llvm::SmallString<256> FullPath = llvm::StringRef(*LocalIndexRoot);
+  llvm::SmallString<256> FullPath = llvm::StringRef(LocalIndexRoot);
   llvm::sys::path::append(FullPath, RelativePath);
   auto Result = URI::createFile(FullPath);
   return Result.toString();
 }
 
 llvm::Expected<std::string> Marshaller::uriToRelativePath(llvm::StringRef URI) {
-  assert(RemoteIndexRoot);
+  assert(!RemoteIndexRoot.empty());
   auto ParsedURI = URI::parse(URI);
   if (!ParsedURI)
     return ParsedURI.takeError();
   if (ParsedURI->scheme() != "file")
     return error("Can not use URI schemes other than file, given: '{0}'.", URI);
   llvm::SmallString<256> Result = ParsedURI->body();
   llvm::StringRef Path(Result);
   // Check for Windows paths (URI=file:///X:/path => Body=/X:/path)
   if (llvm::sys::path::is_absolute(Path.substr(1),
                                    llvm::sys::path::Style::windows))
     Result = Path.drop_front();
-  if (!llvm::sys::path::replace_path_prefix(Result, *RemoteIndexRoot, ""))
+  if (!llvm::sys::path::replace_path_prefix(Result, RemoteIndexRoot, ""))
     return error("File path '{0}' doesn't start with '{1}'.", Result.str(),
-                 *RemoteIndexRoot);
+                 RemoteIndexRoot);
   assert(Result == llvm::sys::path::convert_to_slash(
                        Result, llvm::sys::path::Style::windows));
   return std::string(Result);
 }
 
 clangd::SymbolLocation::Position
 Marshaller::fromProtobuf(const v1::Position &Message) {
   clangd::SymbolLocation::Position Result;

 }
 
 llvm::Expected<clangd::SymbolLocation>
 Marshaller::fromProtobuf(const v1::SymbolLocation &Message) {
   clangd::SymbolLocation Location;
   auto URIString = relativePathToURI(Message.file_path());
   if (!URIString)
     return URIString.takeError();
   Location.FileURI = Strings.save(*URIString).begin();
   Location.Start = fromProtobuf(Message.start());
   Location.End = fromProtobuf(Message.end());
   return Location;
 }
 
 llvm::Expected<v1::SymbolLocation>
 Marshaller::toProtobuf(const clangd::SymbolLocation &Location) {
   remote::v1::SymbolLocation Result;