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