[PATCH] D43246: [clangd] Assert path is absolute when assigning to URIForFile.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: clangd/Protocol.h:52
 
 struct URIForFile {
+  URIForFile() = default;
----------------
ilya-biryukov wrote:
> 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?
> 
> 
I agree that this change would help debugging, so I don't really have objection. I'll leave approval to Sam though, as he had some suggestions.

I wonder if we want to make the assertion more aggressive. Currently, this only helps with assertion on. Would it make sense to also check this in production code and provide useful error message so that it would be easier for us to spot the problem when users report a bug? 


================
Comment at: clangd/Protocol.h:57
+  /// Retrieves absolute path to the file.
+  PathRef getFile() const { return File; }
+
----------------
nit: I like `file()` that Sam suggested a bit more. 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43246