ilya-biryukov added a comment.
In https://reviews.llvm.org/D43246#1006525, @ioeric wrote:
> I think another option to prevent the bug in r325029 from happening would be
> providing a canonical approach for retrieving (absolute) paths from
> `FileManager`. We already have code in symbol collector that does this, and
> we have similar code in XRefs.cpp now. We should probably move them to
> `clangd/Path.h` and share the code?
I agree with you, we should do this.
But this change is not a mitigation for r325029 specifically. URIForFile would
still be tremendously easy to misuse in other cases as well.
================
Comment at: clangd/Protocol.h:52
struct URIForFile {
+ URIForFile() = default;
----------------
sammccall wrote:
> I don't like how the API changes here take us further away from the other
> structs in this file.
> Why does this one enforce invariants, when the others don't?
>
> If we do want to make this more "safe", I'd suggest making it look more like
> a URI string (since that's what its protocol role is), like:
>
> ```
> class LSPURI {
> LSPURI(StringRef URI) { assert if bad }
> static LSPURI ForFile(StringRef AbsPath) { assert if bad }
> operator string() { return URI::createFile(File).toString(); }
> StringRef file() { return AbsFile; }
> operator bool() { return !File.empty(); }
> // move/copy constructor/assignment defaulted
>
> private:
> string AbsFile;
> }
> ```
>
> But the approach Eric suggested does seem promising: more consistent with how
> we handle invariants in protocol structs.
I've updated `URIForFile` to follow the interface you proposed. (Instead of
`operator string()` we have `uri()`, but everything else is there). Haven't
changed the name yet, happy to do it to.
> Why does this one enforce invariants, when the others don't?
It seems useful to catch misbehaving code as early as possible. What are the
other invariants that could be useful?
> But the approach Eric suggested does seem promising: more consistent with how
> we handle invariants in protocol structs.
I agree with Eric's approach, I wasn't trying to fix that specific bug with
this commit.
I'm trying to argue that we should check for non-absolute paths as early as
possible and fail with assertions to make writing code easier in the future. Do
you agree that would be useful or make we shouldn't do it?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D43246
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
