Repository navigation
Question: Is the use of incomplete type correct? #138
Description
The definitions of array_t and object_t in json.hpp look like this:
class basic_json
{
...
using array_t = ArrayType<basic_json, AllocatorType<basic_json>>;
...
using object_t = ObjectType<StringType,
basic_json,
std::less<StringType>,
AllocatorType<std::pair<const StringType,
basic_json>>>;So it is instantiating the ArrayType and the ObjectType templates when basic_json is still an incomplete type. By default those templates are std::vector and std::map.
It seems to work, but I wonder, is it correct according to the standard?
Instantiating STL types with incomplete types seems to be prohibited, so I'd say recursive structures like this would not be technically correct (even if the current implementation allowed it).
From an answer in stackoverflow:
§17.6.4.8 Other functions / 2:
In particular, the effects are undefined in the following cases:
...
- if an incomplete type (3.9) is used as a template argument when instantiating a template component, unless specifically allowed for that component.
In fact, supporting recursive structures is one of the nice features that Boost.Container provides and the STL does not, according to its docs.
What do you think?
Activity
gregmarr commentedon Oct 16, 2015
This is explicitly allowed for vector by N4510 which was accepted for C++17 in May.
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/n4510.html
However, that doesn't really apply here, as this is about an object that has a member variable which is a container of an incomplete type. This doesn't happen here. basic_json contains a union which has has pointers to these containers.
dariomt commentedon Oct 19, 2015
OK, understood.
So if
json_valuehad anobject_tmember instead of anobject_t*then it would be a problem, wouldn't it?I was wondering why the union stored a pointer instead of an object.
The comment there says
stored with pointer to save storagebut it doesn't mention this issue about incomplete types at all.In fact I was wondering about that decision: holding a pointer is a trade-off between having an extra allocation versus having larger
sizeof(json_value).Wouldn't it be better to let the user decide? I for one would prefer slightly larger
json_value's if I can save an extra allocation for storingobject_tandarray_tin the union. That's probably a better trade-off when you have manyobject_t's andarray_t's compared to the total number ofjson_value's.rockeet commentedon Nov 21, 2015
The compilation failed when using sizeof(..) in custom map type, I have encountered this issue, and I removed that sizeof(..) in my custom map, which was used for a compile time safe check. I think the container value type would be better to use a smart pointer instead of object value.
nlohmann commentedon Dec 14, 2015
@dariomt - I could not get the code to compile when adding
object_tandarray_tto the union without pointer.dariomt commentedon Dec 14, 2015
For that you would need to use containers with support for incomplete types, to be able to create recursive structures. Boost.Container offers that support, but it means not using the STL containers :(
In an ideal world, there would be some way to specify if the union should use pointers (for space efficiency) or not use pointers (for memory-allocation efficiency, which would require specifying containers with support for incomplete types).
The default usage would be the current "use pointers in union" with STL containers, but I could easily switch to "not use pointers in the union" and specify Boost containers in the basic_json template.
gregmarr commentedon Dec 14, 2015
As of C++17, std::vector supports incomplete types in containers. This support was not added to std::map yet.
nlohmann commentedon Dec 15, 2015
I close this issue now as there is nothing that can be done at this point.
nlohmann commentedon Dec 15, 2015
(Thanks for all the feedback!)