danielaparker / jsoncons

A C++, header-only library for constructing JSON and JSON-like data formats, with JSON Pointer, JSON Patch, JSON Schema, JSONPath, JMESPath, CSV, MessagePack, CBOR, BSON, UBJSON
https://danielaparker.github.io/jsoncons
Other
699 stars 158 forks source link

fix basic_json_decode_options constructor #437

Closed meierfra-ergon closed 1 year ago

meierfra-ergon commented 1 year ago

I stumbled on this, while trying out jsoncons.
I got the compile error:

/home/meierfra/devel/test/jsonconsTest/jsoncons/include/jsoncons/json_options.hpp: In instantiation of ‘jsoncons::basic_json_decode_options<CharT>::basic_json_decode_options(jsoncons::basic_json_decode_options<CharT>&&) [with CharT = char]’:
/home/meierfra/devel/test/jsonconsTest/main.cpp:31:65:   required from here
/home/meierfra/devel/test/jsonconsTest/jsoncons/include/jsoncons/json_options.hpp:374:102: error: ‘class jsoncons::basic_json_decode_options<char>’ has no member named ‘default_json_parsing’
  374 |         : super_type(std::move(other)), lossless_number_(other.lossless_number_), err_handler_(other.default_json_parsing())
      |                                                                                                ~~~~~~^~~~~~~~~~~~~~~~~~~~

other constructors use 'default_json_parsing' directly, so I think this is what you want.

danielaparker commented 1 year ago

Thanks! It should be other.err_handler(). Could you update your pull request with that change?

meierfra-ergon commented 1 year ago

done.

But I am not sure this is correct either. first I thought we should move the other.errhandler, but then I realized the super_type(std::move(other)) in the initializer_list. Is it even safe to use other after that (for losslessnumber and err_handler)? Either way, would the default move constructor not be a better choice here?

basic_json_decode_options(basic_json_decode_options&& other) = default
danielaparker commented 1 year ago

It's safe. std::move(other) doesn't move anything, it just casts its argument to an rvalue. The base class doesn't touch other.err_handler_.

The reason why we didn't use

basic_json_decode_options(basic_json_decode_options&& other) = default

is because, with multiple inheritance, some supported compilers were unable to generate a default move constructor.

Since this is a move constructor, I should have suggested

std::move(other.err_handler_)

as the argument to err_handler_(), though.

meierfra-ergon commented 1 year ago

thanks for explaining, I updated the PR to use move() to initialize errhandler

danielaparker commented 1 year ago

Thanks for contributing!