[PATCH] D88103: [JSON] Add error reporting facility, used in fromJSON and ObjectMapper.

[Sam McCall via Phabricator via llvm-commits]

sammccall marked 7 inline comments as done.
sammccall added a comment.

Thanks! I've addressed most of the comments one way or the other, please LMK if it's not convincing!
Meanwhile I'm going to split this up to land in parts.



================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:195
+      Root.printErrorContext(Raw, OS);
+      log("{0}", OS.str());
+      // Report the error (e.g. to the client).
----------------
kadircet wrote:
> nit: i would merge with the previous elog, i.e. ` elog("Failed to decode {0} {1}:\n{2}", ... OS.str());`
Oops, this was meant to be a vlog (off by default).


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:198
+      return llvm::make_error<LSPError>(
+          llvm::formatv("failed to decode {0} {1}", PayloadName, PayloadKind),
+          ErrorCode::InvalidParams);
----------------
kadircet wrote:
> why not include `Root.err()` in the failure?
(again, vlog)


================
Comment at: llvm/include/llvm/Support/JSON.h:590
+private:
+  /// One element in a JSON path: an object field (.foo) or array index [27].
+  struct Segment {
----------------
kadircet wrote:
> `.. or a pointer to path::root in case of the "head".`
> 
> the technique is really need, but do we really need all of this compression ? can't we just have
> 
> ```
> struct Segment {
>   union {
>    llvm::StringRef Field;
>    unsigned Index;
>    Root &R;
>   };
>   enum { Object, Array, Head } Type;
> };
> ```
> 
> This would be 24 bytes instead of 16, but hopefulyl will get rid of casts and have some extra type checking ?
Yeah, it probably doesn't matter, but we really are creating a lot of these, and I'm kind of hung up on the idea that this be as close to free as possible.

The casts are really ugly and scattered, I've encapsulated them all into the Segment class which seems much nicer.


================
Comment at: llvm/include/llvm/Support/JSON.h:605
+/// It also stores the latest reported error and the path where it occurred.
+class Path::Root {
+  llvm::StringRef Name;
----------------
kadircet wrote:
> i would could this `Path::Start` or `Path::Head` to emphasize the "begining of the linked list" bit, up to you.
The point of the name is that it's a path within a tree, and this node is the root of the *tree* rather than the list. Rewrote the class comment to reflect this.

(In a linked list it's kind of the tail, right? But the linked-list is an implementation detail)



================
Comment at: llvm/include/llvm/Support/JSON.h:610
+
+  friend Path; // for report().
+
----------------
kadircet wrote:
> nit: I would rather have a `public: void setError(const Path &P, llvm::StringLiteral Message)` but up to you
This is indeed slightly nicer, but this signature loses the path length, so we either have to:
 - eat extra allocations to resize the vector
 - traverse the linked list a third time to get it
 - store it in the path and manage that

I'm not sure the small readability win is worth any of those. I've restricted the friend to just that function.


================
Comment at: llvm/lib/Support/JSON.cpp:319
+  // 'Recurse' is the lambda itself, to allow recursive calls.
+  auto PrintValue = [&](const Value &V, ArrayRef<Segment> Path, auto &Recurse) {
+    auto HighlightCurrent = [&] {
----------------
kadircet wrote:
> nit: I believe this is big enough to be its own function, not sure what we gain by keeping it as a lambda.
It just leaks a bunch of stuff into the header...
We need the separate function to generalize the signature for recursion.
It needs to be a member so it can access Segment, so its signature needs to be in the header.
It can no longer capture `JOS`, so that needs to be a parameter, so we need to forward-declare `OStream` too (or rearrange the header further).

On balance I'm not sure this is a readability win, as the readability of the header is more important.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88103