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

[Sam McCall via Phabricator via llvm-commits]

sammccall added inline comments.


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



Repository:
  rL LLVM

https://reviews.llvm.org/D45753