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

Sam McCall via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed May 2 23:54:53 PDT 2018


sammccall added inline comments.


================
Comment at: include/llvm/Support/JSON.h:355
+
+  // "cheating" move-constructor for moving from initializer_list.
+  ObjectKey(const ObjectKey &&V) {
----------------
Meinersbur wrote:
> sammccall wrote:
> > Meinersbur wrote:
> > > 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?
> > I would also prefer no such hacks, but I can't see a way to get good syntax without the recursive copy, and without the hacks :(
> > Any ideas? Also interested in hearing more opinions on whether this is too hairy to rely on.
> > 
> > That said, I believe this is valid, per all relevant versions of the standard.
> > 
> > > cppreference.com mentions that its elements must be copy-initialized, but only since C++17.
> > I think I'm missing your point here - can you explain why this is good or bad, and what the implication is? It also seems to be a cppreference mistake, the copy-initialized requirement is older.
> > 
> > C++11 says
> > > as if the implementation allocated an array of N elements of type E [...] Each element of that array is copy-initialized
> > i.e. it creates a `T[N]`. We could probably even `const_cast`!
> > 
> > C++14 says
> > > as if the implementation allocated a temporary array of N elements of type **const E** [...]. Each element of that array is copy-initialized [...] The implementation is free to allocate the array in read-only memory **if an explicit array with the same initializer could be so allocated**
> > (emphasis mine). So now the type changes to `const T[N]` (so `const_cast` would be invalid) but mutating mutable members of const objects is allowed, so read-only memory can't be used.
> > 
> > The C++17 language is more obscure
> > > as if the implementation generated and materialized (7.4) a prvalue of type “array of N const E” [...] Each element of that array is copy-initialized
> > but seems to be the same for these purposes.
> Thanks for looking into the standard (note that cppreference mentions C++14, not C++17 as I claimed).
> 
> I understood the standard the following way: If the element is copy-initialized, it should call the copy-constructor. In your implementation, it calls `copyFrom`, i.e. it still makes a recursive copy on each level, meaning the problem is not solved (But you save one by not recursively copying again when constructing from initializer_list). 
> 
> However, I looked into the implementation of initializer_list in libc++ and msvc. It is a list of pointers-to-elements, rather than a flat array of objects. That is, no copy-initialization is not happening, it just points to the already existing elements. Not sure whether this is mandated by the standard.
> I understood the standard the following way: If the element is copy-initialized, it should call the copy-constructor

Ah, this is just the standard being confusing. //copy-initialization// is basically unrelated to //copy-constructors//. Certain syntaxes trigger copy-initialization, and others trigger //direct-initialization//, which behaves slightly differently (copy-initialization won't call `explicit` constructors).

http://en.cppreference.com/w/cpp/language/copy_initialization gives this example, which is relevant here:
```std::string s2 = std::move(s); // this copy-initialization performs a move```


Repository:
  rL LLVM

https://reviews.llvm.org/D45753





More information about the llvm-commits mailing list