[PATCH] D54845: [clangd] Canonicalize file path in URIForFile.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/Protocol.h:73
+
+  /// \p AbsPath is resolved to a canonical path corresponding to its URI.
+  URIForFile(llvm::StringRef AbsPath, llvm::StringRef HintPath = "");
----------------
We need to document the APIs and motivation, these are subtle.


================
Comment at: clangd/Protocol.h:74
+  /// \p AbsPath is resolved to a canonical path corresponding to its URI.
+  URIForFile(llvm::StringRef AbsPath, llvm::StringRef HintPath = "");
+
----------------
I think it's no longer sufficiently clear what this constructor does - it should be a named factory function, and the HintPath should be required.
(If it's easy to implicitly skip canonicalization, then there's no advantage of changing URIForFile vs just asking people to canonicalize the inputs)

e.g. `URIForFile URIForFile::canonicalize(StringRef AbsPath, StringRef HintPath)`


================
Comment at: clangd/Protocol.h:74
+  /// \p AbsPath is resolved to a canonical path corresponding to its URI.
+  URIForFile(llvm::StringRef AbsPath, llvm::StringRef HintPath = "");
+
----------------
sammccall wrote:
> I think it's no longer sufficiently clear what this constructor does - it should be a named factory function, and the HintPath should be required.
> (If it's easy to implicitly skip canonicalization, then there's no advantage of changing URIForFile vs just asking people to canonicalize the inputs)
> 
> e.g. `URIForFile URIForFile::canonicalize(StringRef AbsPath, StringRef HintPath)`
As we're exposing HintPath in more far-flung places, its semantics are becoming less obvious.

I think we should make them more concrete, e.g. rename to `TUPath` and doc like
```
Files can be referred to by several paths (e.g. in the presence of links).
Which one we prefer may depend on where we're coming from.
TUPath is a hint, and should usually be the main entrypoint file we're processing.
```


================
Comment at: clangd/Protocol.h:98
 private:
+  explicit URIForFile(std::string &&File) : File(std::move(File)) {}
+
----------------
Are there really *no* cases where the caller doesn't need canonicalization?


================
Comment at: clangd/URI.cpp:235
+Expected<std::string> URI::resolvePath(StringRef AbsPath, StringRef HintPath) {
+  auto U = create(AbsPath);
+  // "file" scheme doesn't do anything fancy; it would resolve to the same
----------------
this seems pretty indirect - including the call to create(), this function special-cases File 3 times.

Seems a little cleaner to me to remove "file" from the registry, and repeat the loop here. Up to you.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54845