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

Fri May 25 01:45:26 PDT 2018

chandlerc added a comment.

Thanks for the ping!

================
Comment at: include/llvm/Support/JSON.h:62
+/// The string keys may be owned or references.
+class Object : public std::map<ObjectKey, Value> {
+public:
----------------
sammccall wrote:
> sammccall wrote:
> > chandlerc wrote:
> > > I understand the pragmatic reason for this, but I am pretty uncomfortable deriving from standard types like this. I'm worried it will hurt portability due to subtle variations in standard libraries, or subtle changes in standard libraries as we switch to C++N+1 or whatever.
> > > 
> > > I would be much more comfortable using something internal and owning the API you export.
> > > 
> > > Unrelatedly (and not blocking anything), I struggle to believe that std::map is the correct tool for the job... Is it just that DenseMap is a pain to use currently? Is it that these are typically small and not worth a DenseMap? I'm sorry to ask this as I suspect you've already explained this in the original review, but I must admit I'm curious.
> > Are we worried about inheriting the interface of *llvm* types, or just `std` types?
> > 
> > I've switched to inheriting from `DenseMap` here, but I can wrap it if you prefer.
> > (`std::map`'s ordered-ness made `operator==` and `print()` simple, so there's a few more lines now).
> > 
> > `Array` is more complicated: even `SmallVector<Value, 0>` needs `Value`, which sets up a cycle that's hard to break. So I've resorted to wrapping std::vector and exposing a hopefully-sane subset of the API.
> > (Aside: I suspect we *do* want to track e.g. the C++17 changes to `vector`, but that's a burden that shouldn't fall on whoever does the upgrade)
> (And I forgot to mention a new dirty trick here: the explicit use of DenseMapInfo<StringRef> works since ObjectKey can already be implicitly converted back and forth to StringRef. This saves a bunch of unreadable boilerplate which has to go at the *top* of the file, but is admittedly a bit fragile - WDYT?)
> 
I'm somewhat concerned with inheritance generally. I feel like wrapping is just a better pattern and more easily understood and debugged.

The additional code doesn't worry me much.

================
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:
> > 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?

================
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:
> > 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.

Repository:
  rL LLVM

https://reviews.llvm.org/D45753