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