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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Sorry for delays circling back to this.

I think the primary concerns about putting this into the Support library is that we already have one API that is quite similar there: YAMLIO. However, that API is clearly not serving all of the needs of users given that there are mutliple JSON-parsing code paths in the wider project that are already using other libraries (this one, but also Polly). So I think adding this makes lots of sense at this point.

However, I'd like to take this opportunity to try to iterate a bit on the API since it is going into a new, more widely visible home and will almost certainly grow new users as soon as it lands. I'm fairly uncomfortable with the inheritance approach, and I think some of the additional APIs would benefit from (hopefully minor) adjustment. None of this is intended to change the fundamental design in any way though.



================
Comment at: include/llvm/Support/JSON.h:62
+/// The string keys may be owned or references.
+class Object : public std::map<ObjectKey, Value> {
+public:
----------------
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.


================
Comment at: include/llvm/Support/JSON.h:69-71
+  // Allow [] as if Value was default-constructible as null.
+  Value &operator[](const ObjectKey &K);
+  Value &operator[](ObjectKey &&K);
----------------
This does more than what it says. It hides std::map's operator[] overloads, no matter what they are. Is that what you intended?

This is perhaps a good example of why I find inheritance challenging here...


================
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;
----------------
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()`?


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


Repository:
  rL LLVM

https://reviews.llvm.org/D45753