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

[Kadir Cetinkaya via Phabricator via llvm-commits]

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

Thanks! This looks great. I've mostly did the full review anyway but feel free to land in small patches just in case some compiler becomes upset and you need to revert.

Mostly nits, and style related comments. LGTM!



================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:250
                                             ReplyOnce Reply) {
-      Param P;
-      if (fromJSON(RawParams, P)) {
-        (Server.*Handler)(P, std::move(Reply));
-      } else {
-        elog("Failed to decode {0} request.", Method);
-        Reply(llvm::make_error<LSPError>("failed to decode request",
-                                         ErrorCode::InvalidRequest));
-      }
+      if (auto P = parse<Param>(RawParams, Method, "request"))
+        (Server.*Handler)(*P, std::move(Reply));
----------------
nit:
```
auto P = parse ...;
if(!P)
  return Reply(P.takeError());
(Server.*Handler)(*P, std::move(Reply));
```


================
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).
----------------
nit: i would merge with the previous elog, i.e. ` elog("Failed to decode {0} {1}:\n{2}", ... OS.str());`


================
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);
----------------
why not include `Root.err()` in the failure?


================
Comment at: llvm/include/llvm/Support/JSON.h:586
+    return Path(this, Segment{reinterpret_cast<uintptr_t>(Field.data()),
+                              unsigned(Field.size())});
+  }
----------------
static_cast<unsigned>


================
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 {
----------------
`.. 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 ?


================
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;
----------------
i would could this `Path::Start` or `Path::Head` to emphasize the "begining of the linked list" bit, up to you.


================
Comment at: llvm/include/llvm/Support/JSON.h:610
+
+  friend Path; // for report().
+
----------------
nit: I would rather have a `public: void setError(const Path &P, llvm::StringLiteral Message)` but up to you


================
Comment at: llvm/include/llvm/Support/JSON.h:624
+
+  void printErrorContext(const Value &, llvm::raw_ostream &) const;
+};
----------------
would be nice to have some comments with example output


================
Comment at: llvm/lib/Support/JSON.cpp:245
+
+static std::vector<const Object::value_type *> sortedElements(const Object &O) {
+  std::vector<const Object::value_type *> Elements;
----------------
nit: drop static, already in anon namespace


================
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 = [&] {
----------------
nit: I believe this is big enough to be its own function, not sure what we gain by keeping it as a lambda.


================
Comment at: llvm/lib/Support/JSON.cpp:325
+      abbreviateChildren(V, JOS, OS);
+      return;
+    };
----------------
nit: either drop return or `return abbreviateChildren..`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88103