[clangd] Simplify the callside of URI::resolve, NFC.

[hokein]

Summary

• Add a overrloded URI::resolve, which accepts a string URI;
• also fixed some callside that don't check the error;

[hokein]

remove an accident change.

[kadircet]

LGTM, thanks!

clang-tools-extra/clangd/FindSymbols.cpp

48 ↗

(On Diff #221320)

return make_string_error(... ?

Also the error string regarding CD.FileURI should be contained in Path.takeError maybe just print:
Could not resolve path for symbol '{0}': {1}, Sym.Name, Path.takeError() ?

clang-tools-extra/clangd/URI.cpp

190 ↗

(On Diff #221320)

why not just Uri.takeError ?

same below for Path.takeError

clang-tools-extra/clangd/URI.h

66 ↗

(On Diff #221320)

what about:

Same as above, in addition parses \p FileURI using URI::parse to create a URI.

clang-tools-extra/clangd/index/BackgroundIndexLoader.cpp

30 ↗

(On Diff #221320)

wow, thanks for catching this one.

Failed to resolve URI {0}: {1}

[kadircet]

clang-tools-extra/clangd/index/BackgroundIndexLoader.cpp

30 ↗

(On Diff #221320)

actually can you just inline this into call site?

[hokein]

address comments.

[hokein]

clang-tools-extra/clangd/FindSymbols.cpp

48 ↗

(On Diff #221320)

make_string_error is an internal helper in URI.cpp only.

clang-tools-extra/clangd/URI.cpp

190 ↗

(On Diff #221320)

good point. The code was changed back and forth a few times.

[kadircet]

LGTM, thanks!

clang-tools-extra/clangd/FindSymbols.cpp

48 ↗

(On Diff #221320)

ah I see, when I searched for make_string_error, there were too many references so I assumed it was a generic helper, but apparently every file has an internal implementation ...

Whatever that can be an adventure for another day...

clang-tools-extra/trunk/clangd/CodeComplete.cpp

if (C.IdentifierResult) {
       Completion.Origin |= SymbolOrigin::Identifier;
       Completion.Kind = CompletionItemKind::Text;
       Completion.Name = C.IdentifierResult->Name;
     }
 
     // Turn absolute path into a literal string that can be #included.
     auto Inserted = [&](llvm::StringRef Header)
         -> llvm::Expected<std::pair<std::string, bool>> {
-      auto DeclaringURI =
-          URI::parse(C.IndexResult->CanonicalDeclaration.FileURI);
-      if (!DeclaringURI)
-        return DeclaringURI.takeError();
-      auto ResolvedDeclaring = URI::resolve(*DeclaringURI, FileName);
+      auto ResolvedDeclaring =
+          URI::resolve(C.IndexResult->CanonicalDeclaration.FileURI, FileName);
       if (!ResolvedDeclaring)
         return ResolvedDeclaring.takeError();
       auto ResolvedInserted = toHeaderFile(Header, FileName);
       if (!ResolvedInserted)
         return ResolvedInserted.takeError();
       auto Spelled = Includes.calculateIncludePath(*ResolvedInserted, FileName);
       if (!Spelled)
         return llvm::createStringError(llvm::inconvertibleErrorCode(),

clang-tools-extra/trunk/clangd/FindSymbols.cpp

 };
 
 } // namespace
 
 llvm::Expected<Location> symbolToLocation(const Symbol &Sym,
                                           llvm::StringRef HintPath) {
   // Prefer the definition over e.g. a function declaration in a header
   auto &CD = Sym.Definition ? Sym.Definition : Sym.CanonicalDeclaration;
-  auto Uri = URI::parse(CD.FileURI);
-  if (!Uri) {
-    return llvm::make_error<llvm::StringError>(
-        formatv("Could not parse URI '{0}' for symbol '{1}'.", CD.FileURI,
-                Sym.Name),
-        llvm::inconvertibleErrorCode());
-  }
-  auto Path = URI::resolve(*Uri, HintPath);
+  auto Path = URI::resolve(CD.FileURI, HintPath);
   if (!Path) {
     return llvm::make_error<llvm::StringError>(
-        formatv("Could not resolve path for URI '{0}' for symbol '{1}'.",
-                Uri->toString(), Sym.Name),
+        formatv("Could not resolve path for symbol '{0}': {1}",
+                Sym.Name, llvm::toString(Path.takeError())),
         llvm::inconvertibleErrorCode());
   }
   Location L;
   // Use HintPath as TUPath since there is no TU associated with this
   // request.
   L.uri = URIForFile::canonicalize(*Path, HintPath);
   Position Start, End;
   Start.line = CD.Start.line();

clang-tools-extra/trunk/clangd/IncludeFixer.cpp

if (!Matched.IncludeHeaders.empty() && Matched.Definition &&
       Fixes = fixesForSymbols(Syms);
   }
   return Fixes;
 }
 
 std::vector<Fix> IncludeFixer::fixesForSymbols(const SymbolSlab &Syms) const {
   auto Inserted = [&](const Symbol &Sym, llvm::StringRef Header)
       -> llvm::Expected<std::pair<std::string, bool>> {
-    auto DeclaringURI = URI::parse(Sym.CanonicalDeclaration.FileURI);
-    if (!DeclaringURI)
-      return DeclaringURI.takeError();
-    auto ResolvedDeclaring = URI::resolve(*DeclaringURI, File);
+    auto ResolvedDeclaring =
+        URI::resolve(Sym.CanonicalDeclaration.FileURI, File);
     if (!ResolvedDeclaring)
       return ResolvedDeclaring.takeError();
     auto ResolvedInserted = toHeaderFile(Header, File);
     if (!ResolvedInserted)
       return ResolvedInserted.takeError();
     auto Spelled = Inserter->calculateIncludePath(*ResolvedInserted, File);
     if (!Spelled)
       return llvm::createStringError(llvm::inconvertibleErrorCode(),

clang-tools-extra/trunk/clangd/URI.h

public:
   /// Resolves the absolute path of \p U. If there is no matching scheme, or the
   /// URI is invalid in the scheme, this returns an error.
   ///
   /// \p HintPath A related path, such as the current file or working directory,
   /// which can help disambiguate when the same file exists in many workspaces.
   static llvm::Expected<std::string> resolve(const URI &U,
                                              llvm::StringRef HintPath = "");
 
+  /// Same as above, in addition it parses the \p FileURI using URI::parse.
+  static llvm::Expected<std::string> resolve(llvm::StringRef FileURI,
+                                             llvm::StringRef HintPath = "");
+
   /// Resolves \p AbsPath into a canonical path of its URI, by converting
   /// \p AbsPath to URI and resolving the URI to get th canonical path.
   /// This ensures that paths with the same URI are resolved into consistent
   /// file path.
   static llvm::Expected<std::string> resolvePath(llvm::StringRef AbsPath,
                                                  llvm::StringRef HintPath = "");
 
   /// Gets the preferred spelling of this file for #include, if there is one,

clang-tools-extra/trunk/clangd/URI.cpp

if (Uri.consume_front("//")) {
     Pos = Uri.find('/');
     U.Authority = percentDecode(Uri.substr(0, Pos));
     Uri = Uri.substr(Pos);
   }
   U.Body = percentDecode(Uri);
   return U;
 }
 
+llvm::Expected<std::string> URI::resolve(llvm::StringRef FileURI,
+                                         llvm::StringRef HintPath) {
+  auto Uri = URI::parse(FileURI);
+  if (!Uri)
+    return Uri.takeError();
+  auto Path = URI::resolve(*Uri, HintPath);
+  if (!Path)
+    return Path.takeError();
+  return *Path;
+}
+
 llvm::Expected<URI> URI::create(llvm::StringRef AbsolutePath,
                                 llvm::StringRef Scheme) {
   if (!llvm::sys::path::is_absolute(AbsolutePath))
     return make_string_error("Not a valid absolute path: " + AbsolutePath);
   auto S = findSchemeByName(Scheme);
   if (!S)
     return S.takeError();
   return S->get()->uriFromAbsolutePath(AbsolutePath);

clang-tools-extra/trunk/clangd/index/Background.cpp

 // Resolves URI to file paths with cache.
 class URIToFileCache {
 public:
   URIToFileCache(llvm::StringRef HintPath) : HintPath(HintPath) {}
 
   llvm::StringRef resolve(llvm::StringRef FileURI) {
     auto I = URIToPathCache.try_emplace(FileURI);
     if (I.second) {
-      auto U = URI::parse(FileURI);
-      if (!U) {
-        elog("Failed to parse URI {0}: {1}", FileURI, U.takeError());
-        assert(false && "Failed to parse URI");
-        return "";
-      }
-      auto Path = URI::resolve(*U, HintPath);
+      auto Path = URI::resolve(FileURI, HintPath);
       if (!Path) {
         elog("Failed to resolve URI {0}: {1}", FileURI, Path.takeError());
         assert(false && "Failed to resolve URI");
         return "";
       }
       I.first->second = *Path;
     }
     return I.first->second;

clang-tools-extra/trunk/clangd/index/BackgroundIndexLoader.cpp

 #include <string>
 #include <utility>
 #include <vector>
 
 namespace clang {
 namespace clangd {
 namespace {
 
-llvm::Optional<Path> uriToAbsolutePath(llvm::StringRef URI, PathRef HintPath) {
-  auto U = URI::parse(URI);
-  if (!U)
-    return llvm::None;
-  auto AbsolutePath = URI::resolve(*U, HintPath);
-  if (!AbsolutePath)
-    return llvm::None;
-  return *AbsolutePath;
-}
-
 /// A helper class to cache BackgroundIndexStorage operations and keep the
 /// inverse dependency mapping.
 class BackgroundIndexLoader {
 public:
   BackgroundIndexLoader(BackgroundIndexStorage::Factory &IndexStorageFactory)
       : IndexStorageFactory(IndexStorageFactory) {}
   /// Load the shards for \p MainFile and all of its dependencies.
   void load(PathRef MainFile);
BackgroundIndexLoader::loadShard(PathRef StartSourceFile, PathRef DependentTU) {
   auto Shard = Storage->loadShard(StartSourceFile);
   if (!Shard || !Shard->Sources) {
     vlog("Failed to load shard: {0}", StartSourceFile);
     return {LS, Edges};
   }
 
   LS.Shard = std::move(Shard);
   for (const auto &It : *LS.Shard->Sources) {
-    auto AbsPath = uriToAbsolutePath(It.getKey(), StartSourceFile);
-    if (!AbsPath)
+    auto AbsPath = URI::resolve(It.getKey(), StartSourceFile);
+    if (!AbsPath) {
+      elog("Failed to resolve URI: {0}", AbsPath.takeError());
       continue;
+    }
     // A shard contains only edges for non main-file sources.
     if (*AbsPath != StartSourceFile) {
       Edges.push_back(*AbsPath);
       continue;
     }
 
     // Fill in shard metadata.
     const IncludeGraphNode &IGN = It.getValue();