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

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Thanks for the review! PTAL



================
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:
> 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.
> ```
Sounds good. Thanks!


================
Comment at: clangd/Protocol.h:98
 private:
+  explicit URIForFile(std::string &&File) : File(std::move(File)) {}
+
----------------
sammccall wrote:
> Are there really *no* cases where the caller doesn't need canonicalization?
As chatted offline, there seems to be no cases where we don
t need canonicalization. We didn't do this in unit test, but we should do via matchers.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54845