[PATCH] D87335: [json] Create some llvm::Expected-based accessors

[Greg Clayton via Phabricator via llvm-commits]

clayborg added a comment.

Right now all users of this code end up creating little extra functions that must work around this. lldb-vscode has it's own methods, the new IntelPT will do its own thing.

I think these APIs make sense for the API itself as a user of the API from my lldb-vscode work. Can we iterate on this and come up with a solution?

In D87335#2282034 <https://reviews.llvm.org/D87335#2282034>, @sammccall wrote:

> This is a sensible thing to want and very nice code.
> I'm not sure about putting it in JSON.h - there are a few unfortunate aspects that I just don't know how to fix, but limit the benefit which has to be measured against the cost of adding more complexity to the API.
>
> - the error messages aren't *quite* good enough to be directly user-visible in the general case. e.g. "JSON object missing the 'id' field" - some hint to which (sub)object is often needed.

I believe they are good enough, they say 'id' is missing, but then print the full JSON object that they are missing from right?

> - because the getAsX() method names are taken, the new accessors are stuck with awkward names

See my "expect" suggestion in inline comments?

> - Expected<pointer> (for object/array) is an awkward type, because -> doesn't really work and the meaning/impossibility of nullptr isn't obvious.

Since this we are using Expected can we change the value to be "Object&" or "Array &" instead of a pointer?

> - this is useful when mapping objects with many fields, but it's not clear how to extend it to ObjectMapper or fromJSON (which maybe themselves don't have the best design, but...)



> I think the first concern is the most important - providing an error-bearing API whose error messages aren't actually good enough for a lot of purposes is a bit of a hazard.
> To give good errors you really need to pass around some context, but this cuts down on the simplicity of the API.

Most of these errors are when we are trying to extract a key/value pair from an Object. So I don't really think we need to pass around more context? to make things make sense.

> Happy to discuss this further if we want to try to come up with a general solution (I know clangd currently reports no errors other than "this is invalid", so I have some interest!)
> Otherwise it might be best to carry these as non-member functions in the local project that can make the tradeoffs around APIs. Since they're implemented entirely on top of the public API this seems manageable, just a little ugly :-\

I would like to see more consistent use of this API with common errors instead of everyone make up their own error messages.



================
Comment at: llvm/include/llvm/Support/JSON.h:152-158
+  llvm::Expected<const json::Value *> getOrError(StringRef K) const;
+  llvm::Expected<int64_t> getIntegerOrError(StringRef K) const;
+  llvm::Expected<llvm::StringRef> getStringOrError(StringRef K) const;
+  llvm::Expected<llvm::Optional<llvm::StringRef>>
+  getOptionalStringOrError(StringRef K) const;
+  llvm::Expected<const json::Object *> getObjectOrError(StringRef K) const;
+  llvm::Expected<const json::Array *> getArrayOrError(StringRef K) const;
----------------
Maybe instead of these start with "get" and ending with "OrError", we could just name these "expect...". The getOptionalStringOrError doesn't map well to this though


================
Comment at: llvm/include/llvm/Support/JSON.h:459-462
+  llvm::Expected<const json::Object *> getAsObjectOrError() const;
+  llvm::Expected<const json::Array *> getAsArrayOrError() const;
+  llvm::Expected<llvm::StringRef> getAsStringOrError() const;
+  llvm::Expected<int64_t> getAsIntegerOrError() const;
----------------
switch over to "expect" as mentioned above?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87335/new/

https://reviews.llvm.org/D87335