[PATCH] D40399: [clangd] Add missing (but documented!) JSONExpr typed accessors

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/JSONExpr.h:368
+    // Typed accessors return None/nullptr if the element has the wrong type.
+    llvm::Optional<std::nullptr_t> null(size_t I) const {
+      return (*this)[I].null();
----------------
ioeric wrote:
> Why is this needed? `v[x].null()` seems to be more intuitive than `v.null(x)`.
In the overwhelmingly common case when parsing, v is a pointer, because it resulted from a call to array().

So it's `(*v)[x].null()` vs `v->null(x)` - I think the latter is slightly more readable.

But more compelling to me is having consistency between object/array.

We *can* live without these if you like, though - most of the time we iterate over arrays rather than indexed access. 


================
Comment at: unittests/clangd/JSONExprTests.cpp:203
+
+  EXPECT_FALSE(O->null("missing"));
+  EXPECT_FALSE(O->null("boolean"));
----------------
ioeric wrote:
> It's not very obvious that this accesses a KV and converts the value, by only reading this line. Would it make sense to make the APIs more explicit e.g. `get_as_xxx(...)`? 
I think this is made worse because it's an artificial example.
I've sent D40406 which has "real life" code - what do you think about the getters there?

I think the best variant names would be `asString()` or `getString()`.

I'm a bit reluctant to add a fixed prefix to these common accessors as they seem noisy. I also think we'd need to change all the names on Expr(), which seems like a shame.

So I'm not sure about changing this, but curious what you think of the Protocol conversion code.


https://reviews.llvm.org/D40399