sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

LG if we want to do this
(please getFile -> file though!)



================
Comment at: clangd/Protocol.h:52
 
 struct URIForFile {
+  URIForFile() = default;
----------------
ioeric wrote:
> 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? 
I'm not going to block this change, but I somewhat prefer the current code 
without it (and assertions in the places that populate URIForFile instead).

> It seems useful to catch misbehaving code as early as possible. What are the 
> other invariants that could be useful?

At a quick scan of `protocol.h` from the top:

Position.line/character >= 0
Range.start <= Range.end
Location.uri non-empty
TextDocumentItem.uri non empty (and lots of other uris)
FormattingOptions.tabSize >= 0
Diagnostic.message non-empty
ExecuteCommandParams.command has a valid value
...



================
Comment at: clangd/Protocol.h:28
 #include "JSONExpr.h"
+#include "Path.h"
 #include "URI.h"
----------------
can we hold off on adding `Path` to more places in this patch?
I'd like to see if others find it useful - it mostly seems obscure to me.


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

Reply via email to