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