Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Question: Is the use of incomplete type correct? #138

Closed
dariomt opened this issue Oct 16, 2015 · 8 comments
Closed

Question: Is the use of incomplete type correct? #138

dariomt opened this issue Oct 16, 2015 · 8 comments

Comments

@dariomt
Copy link
Contributor

dariomt commented Oct 16, 2015

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?

@gregmarr
Copy link
Contributor

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
Copy link
Contributor Author

dariomt commented Oct 19, 2015

OK, understood.
So if json_value had an object_t member instead of an object_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 storage but 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 storing object_t and array_t in the union. That's probably a better trade-off when you have many object_t's and array_t's compared to the total number of json_value's.

@rockeet
Copy link

rockeet commented 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
Copy link
Owner

@dariomt - I could not get the code to compile when adding object_t and array_t to the union without pointer.

@dariomt
Copy link
Contributor Author

dariomt commented 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
Copy link
Contributor

As of C++17, std::vector supports incomplete types in containers. This support was not added to std::map yet.

@nlohmann
Copy link
Owner

I close this issue now as there is nothing that can be done at this point.

@nlohmann
Copy link
Owner

(Thanks for all the feedback!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants