glideapps / quicktype

Generate types and converters from JSON, Schema, and GraphQL
https://app.quicktype.io
Apache License 2.0
12.14k stars 1.06k forks source link

Use `std::optional` for optional objects in C++17 #1687

Open TerensTare opened 3 years ago

TerensTare commented 3 years ago

I just tried quicktype for parsing schema supporting optional fields in C++, and noticed it uses std::shared_ptr for these fields. C++17 introduced std::optional which is closer than std::shared_ptr to the "optional value" notion, hence I think it should be used on demand instead of std::shared_ptr. Furthermore, std::optional doesn't need to allocate memory on nlohmann::adl_serializer<std::optional<T>>::from_json as oposed to std::shared_ptr. There is already a Require a dependency on boost. Without boost, C++17 is required switch, so the changes to implement should be relatively small.

TerensTare commented 3 years ago

Browsing the source code of quicktype, I found a note explaining why boost/std optional aren't the best choice for usage with Json. I still think std::optional is a better choice here because it avoids dynamic allocation and isn't required to offer thread-safe operations, which result in extra overhead when not needed. Hence, I think it would be better if quicktype's interface would offer an option to use optional or shared_ptr as needed: When one is sure no forward-declared data type exists and/or don't want thread safety by default, they can enable the "use optional" flag. Otherwise, fall back to shared_ptr. For compatibility, I think this flag should be off by default (aka. use shared_ptr by default).

oiwg commented 2 years ago

Probably a good idea for someone with sufficient privileges to add the "C++" label to this issue! :)

@TerensTare I'd be interested to know if you experimented with this at all, and found any pitfalls / successes!

TerensTare commented 2 years ago

@oiwg I haven't been working on this issue since my last comment, so that comment is what describes my experience on the topic the best.

SimonCahill commented 1 year ago

This is currently being implemented. I've created a discussion thread and there was a PR concerning the issue. Currently still looking for it.

SimonCahill commented 1 year ago

Here it is :)