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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: include/llvm/Support/JSON.h:355
+
+  // "cheating" move-constructor for moving from initializer_list.
+  ObjectKey(const ObjectKey &&V) {
----------------
sammccall wrote:
> Meinersbur wrote:
> > Can you elaborate on why this is needed? AFAIK `std::initializer_list`s are not meant to be moved from.
> I was able to mitigate this, eliminating the const-rvalue-reference constructors. (This optimization isn't important for ObjectKey, and for Object I friended the relevant classes instead.)
> 
> The reasoning here is roughly:
>  - we need some syntax to support many KV pairs for an object, or elements in an array
>  - function syntax fails because it doesn't format well in long-list cases. Clang-format does a better job if lists are braced lists, in particular it offers you the ability to force one-per-line with a trailing comma in the list.
>  - a variadic constructor fails because this must be templated on the arg type, which means args can't be braced list expressions themselves, as those do not have a deducible type. This would hurt map-like object-literal syntax...
>  - so we're left with the `std::initializer_list` constructor as the way to pass variable numbers of arguments, in a way that formats nicely, and allows them to be coerced to a chosen type. However `initializer_list` acts like a container of `const<T>`, which would mean naively json::Value{{{{{1}}}}} would result in a deep copy at every level of nesting. Fortunately the standard spells out enough of how the contents of init-lists are constructed that moving the data out of them seems well-defined.
> 
> I'm not sure how much of this stuff belongs in the comments here - it's more design doc than user guide.
`std::initializer_list` acting like a container of const elements is probably for a reason. I'd prefer no such hacks, but also see that endless copying of elements might justify cheating.

[[ http://en.cppreference.com/w/cpp/utility/initializer_list | cppreference.com ]] mentions that its elements must be copy-initialized, but only since C++17.

does anyone else have an opinion on this?


Repository:
  rL LLVM

https://reviews.llvm.org/D45753