[PATCH] D45753: Lift JSON library from clang-tools-extra/clangd to llvm/Support.

[Sam McCall via Phabricator via llvm-commits]

sammccall added a comment.

OK, i'm done being recalcitrant, I think everything is addressed now :-)



================
Comment at: include/llvm/Support/JSON.h:100
+
+  // Typed accessors return None/nullptr if the element has the wrong type.
+  llvm::Optional<std::nullptr_t> getNull(size_t I) const;
----------------
sammccall wrote:
> chandlerc wrote:
> > sammccall wrote:
> > > chandlerc wrote:
> > > > Do you expect users to primarily use these typed accessors? Or the underlying vector?
> > > > 
> > > > Should they take iterators instead of indices?
> > > > 
> > > > These seem to make the API somewhat hostile to range based for loops. I'm surprised this isn't more a facility provided by the iterator...
> > > > 
> > > > I find the name somewhat confusing too -- see my comment below for some clarity of why.
> > > > 
> > > > But what's the advantage here? Why not just `arr[i].asNull()`?
> > > I expect the most common access patterns to be:
> > > 1. mapping to a `vector<Something>` using `fromJSON`
> > > 2. iterating over `Value`s using ranged-for (followed by `asFoo()` on each element)
> > > 3. these `getFoo(I)` accessors
> > > 
> > > They're not nearly as useful/important as the typed accessors on Object. The benefit is:
> > > - symmetry with Object makes the API easier to follow (thus `size_t` rather than `iterator`)
> > > - convenient when particular indices have semantics (like `executeCommand` parameter list in LSP), rather than being a homogeneous collection
> > > - more readable than `operator[]` when calling on a pointer, which is common due to the `if (Array* A = V.asArray())` idiom - `(*A)[i].asNull()` vs `A->getNull(i)`.
> > > 
> > > > These seem to make the API somewhat hostile to range based for loops. I'm surprised this isn't more a facility provided by the iterator...
> > > 
> > > Can you elaborate on what kind of loop you'd like to write? I tend to find clever iterator APIs fairly  undiscoverable/obscure, but I'm not sure what I'm missing.
> > Definitely not advocating for more clever iterator API. Mostly pointing out that index-oriented APIs are particularly hard to use with range based for loops. Whereas, using `foo[i].asNull()` is fine because the indexing is orthogonal to the query and can be replaced with something more range-based or iterator based if useful.
> > 
> > If the issue is just that operator[] is annoying with pointers, how about just a method for indexing so that you can use `->` to call it? `A->at(i).asNull()` or `A->get(i).asNull()` seem fine?
> The `Object` case is different and more important, and worth resolving first.
> 
> `Object::getNumber(StringRef)` returns a number if the property exists **and** is a number. Dropping this accessor and writing `if (auto N = O->get("foo").getAsNumber()) ...` isn't viable as it crashes if the property is missing. Instead you'd have to write `if (auto MN = O->get("foo")) if (auto N = MN->getAsNumber()) ...`. This is the overwhelmingly common pattern for parsing objects, and I do think we should support this in one expression and preferably one call.
> 
> If you disagree with that, let's talk about that first :-)
> 
> > If the issue is just that operator[] is annoying with pointers, how about just a method for indexing so that you can use -> to call it? A->at(i).asNull() or A->get(i).asNull() seem fine?
> 
> So assuming we're going to have these methods for object, I think the biggest issue is consistency with object. I do also think `A->getNumber(I)` is a lot nicer than `A->get(I).getAsNumber()`. But neither of these are hard objections and I'm happy to just drop all of the `Array::get*`.
I've dropped the typed `Array::get*`, but kept `Object::get*` as explained above.

We now have this glorious code in the test: `(*(*A)[4].getAsArray())[1].getAsInteger()` but that's not terribly representative of likely real-world use.


================
Comment at: include/llvm/Support/JSON.h:266
+
+  // Typed accessors return None/nullptr if the Value is not of this type.
+  llvm::Optional<std::nullptr_t> asNull() const {
----------------
sammccall wrote:
> chandlerc wrote:
> > sammccall wrote:
> > > chandlerc wrote:
> > > > A more conventional name would be `getAsFoo` matching `getAs<T>`.
> > > > 
> > > > Also, is it really worth having all of these? `getAs<bool>` and `getAs<double>` seem just as nice as the non-templated versions to me... But I guess `getAsString` and `getAsInteger` do interesting validation and such.
> > > > A more conventional name would be getAsFoo matching getAs<T>.
> > > Sure, in Java ;-)
> > > Adding "get" because a function must have a verb seems like cargo culting to me - we're trying to signal the effects, but there aren't any! Maybe it's my exposure, but I don't see any ambiguity as to what `asBoolean()` might do.
> > > 
> > > I'd like to propose a style guide change to be more like https://swift.org/documentation/api-design-guidelines/#strive-for-fluent-usage here. Meanwhile, I can change this to match the style guide as it stands if you think this is important. I do think it hurts the signal/noise.
> > > 
> > > > Also, is it really worth having all of these? getAs<bool> and getAs<double> seem just as nice as the non-templated versions to me... 
> > > 
> > > I do prefer the non-templated version:
> > >  - `getAs<bool>` etc isn't fewer things to understand, and it's not shorter, it's just fewer distinct tokens. I'm not sure what we're trying to conserve here.
> > >  - the non-templated functions are more discoverable: easier to read in the header, and work better with code completion, and easier to search for
> > >  - `getAsNumber` is a better name than `getAs<double>`, as the relevant concept here is JSON's "Number" not the types used to represent them in C++. (this is "interesting validation and such" I think)
> > >  - I don't want people to think they can write `getAs<int>()`, and if they do I want the error message to be easy to understand. Templates make both of these harder.
> > > 
> > I find the arguments against templates compelling FWIW.
> > 
> > That said, while I actually support not using `get` superfluously in many cases, I don't actually like it here. There is an interesting and potentially complex conversion happening, and I personally much prefer `getAsFoo()` for that. The only time I'm really quite happy omitting the `get` is when it is truly just an accessor and not providing any "interesting" logic (a higher bar than the Swift convention you cite in the llvm-dev thread).
> > 
> > Anyways, for now, I suggest the `getAsNumber` pattern.
> OK. I'd really like to avoid three-word names here so I'll have a think about this over the weekend.
> 
> I do think `get` is always superfluous - if you want to signal that something tricky is happening, `get` doesn't do that.
> 
> This is logically just accessing a member of a discriminated union, which I don't think is anything tricky at all, so I'm not sure what the verb should be. But I'll find something.
I tried a few out with a small survey (N=4). From most to least preferred
 - `asNumber()` - this was most people's favorite
 - `getAsNumber()` - everyone could live with this; one person liked it as much as `asNumber`
 - `getNumber()` - an awkward compromise
 - `toNumber()` - probably sounds too much like a conversion
 - `number()` - I really like this, but nobody else does

I've changed them to `getAsNumber`, but I'm still a bit conflicted here.


Repository:
  rL LLVM

https://reviews.llvm.org/D45753