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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
