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

fix basic_json_decode_options constructor #437

Conversation

meierfra-ergon
Copy link
Contributor

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.

@meierfra-ergon meierfra-ergon marked this pull request as ready for review July 11, 2023 14:14
@danielaparker
Copy link
Owner

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

@meierfra-ergon meierfra-ergon force-pushed the fix_basic_json_decode_options_constructor branch from d458ea3 to a8c2b59 Compare July 11, 2023 16:25
@meierfra-ergon
Copy link
Contributor Author

meierfra-ergon commented Jul 11, 2023

done.

But I am not sure this is correct either. first I thought we should move the other.err_handler_, but then I realized the super_type(std::move(other)) in the initializer_list. Is it even safe to use other after that (for lossless_number_ 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
Copy link
Owner

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 meierfra-ergon force-pushed the fix_basic_json_decode_options_constructor branch from a8c2b59 to 0eebd02 Compare July 12, 2023 11:36
@meierfra-ergon
Copy link
Contributor Author

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

@danielaparker danielaparker merged commit 5ad0b06 into danielaparker:master Jul 12, 2023
@danielaparker
Copy link
Owner

Thanks for contributing!

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

Successfully merging this pull request may close these issues.

2 participants