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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks for the review! I think you're right about new names.



================
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();
----------------
sammccall wrote:
> 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. 
I've kept these for now, for readability and symmetry.


================
Comment at: unittests/clangd/JSONExprTests.cpp:203
+
+  EXPECT_FALSE(O->null("missing"));
+  EXPECT_FALSE(O->null("boolean"));
----------------
sammccall wrote:
> 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.
After offline discussion, renamed to `Expr::asString()` and `obj::getString()`.
These aren't much longer and are more accessible to casual inspection.


https://reviews.llvm.org/D40399