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