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 'const' qualifier on bool& has no effect #3678

Merged
merged 2 commits into from
Aug 7, 2022

Conversation

falbrechtskirchinger
Copy link
Contributor

Disable the to_json overload for std::vector<bool>::reference when std::vector<bool>::reference is the same as basic_json::boolean_t&.

Thanks, @georgthegreat, for pointing out this issue.

Closes #3677.

@coveralls
Copy link

coveralls commented Aug 5, 2022

Coverage Status

Coverage remained the same at 100.0% when pulling e90dc44 on falbrechtskirchinger:fix-bool-ref into b0422f8 on nlohmann:develop.

@nlohmann
Copy link
Owner

nlohmann commented Aug 5, 2022

Do we have a chance to add a regression test?

@falbrechtskirchinger
Copy link
Contributor Author

falbrechtskirchinger commented Aug 5, 2022

Not without external dependencies. And the warning flag seems to be new.

Edit: The warning isn't new, just poorly searchable.

@georgthegreat
Copy link

This does not work, unfortunately.
clang12 still issues a warning when he sees const bool&:

include/nlohmann/detail/conversions/to_json.hpp:274:39: error: 'const' qualifier on reference type 'std::vector<bool>::reference' (aka 'bool &') has no effect [-Werror,-Wignored-qualifiers]
inline void to_json(BasicJsonType& j, const std::vector<bool>::reference& b) noexcept

@falbrechtskirchinger
Copy link
Contributor Author

Oh, that's because I made at least one mistake.

@georgthegreat
Copy link

Still no

include/nlohmann/detail/conversions/to_json.hpp:272:36: error: 'const' qualifier on reference type 'std::vector<bool>::reference' (aka 'bool &') has no effect [-Werror,-Wignored-qualifiers]
               std::is_convertible<const std::vector<bool>::reference&, typename BasicJsonType::boolean_t>::value
                                   ^~~~~~
include/nlohmann/detail/conversions/to_json.hpp:274:39: error: 'const' qualifier on reference type 'std::vector<bool>::reference' (aka 'bool &') has no effect [-Werror,-Wignored-qualifiers]
inline void to_json(BasicJsonType& j, const std::vector<bool>::reference& b) noexcept

@falbrechtskirchinger
Copy link
Contributor Author

falbrechtskirchinger commented Aug 5, 2022

I could try flipping those checks, hoping the compiler short-circuits and doesn't get to the is_convertible check which seems to trigger the warning.

Edit: No. It would still trigger on the function prototype. I'll try something else.

@georgthegreat
Copy link

georgthegreat commented Aug 5, 2022

And why const_reference fix proposed #3677 would not work?

UPD: I see, almost every CI build is broken.

@falbrechtskirchinger
Copy link
Contributor Author

In the end, it would still conflict with the existing overload.

@falbrechtskirchinger
Copy link
Contributor Author

One more try. I'm grasping at straws with this one. If it doesn't work, we may just have to suppress the warning for that function.

@georgthegreat
Copy link

georgthegreat commented Aug 5, 2022

It looks like the latest version will work for us.

Thanks, @falbrechtskirchinger!

@gregmarr
Copy link
Contributor

gregmarr commented Aug 5, 2022

Probably not a bad idea to also do the change from 3677 and use const_reference.

@falbrechtskirchinger
Copy link
Contributor Author

falbrechtskirchinger commented Aug 5, 2022

No, that doesn't work. ::reference and ::const_reference are different types. In fact, ::const_reference is bool in stdlibc++.

Assuming my last fix attempt works, I was going to give ::const_reference the same treatment.

Edit: Missed @georgthegreat's reply. Good to hear. I will make the change then,

@falbrechtskirchinger falbrechtskirchinger force-pushed the fix-bool-ref branch 2 times, most recently from f5188df to 982edd9 Compare August 5, 2022 19:39
@falbrechtskirchinger
Copy link
Contributor Author

I decided against dealing with std::vector<bool>::const_reference for now. Chances are, I'd make matters worse for everyone instead of fixing things for some hypothetical STL.

@github-actions github-actions bot added the tests label Aug 5, 2022
@falbrechtskirchinger
Copy link
Contributor Author

And it turns out, that libc++ does use a different type for const_reference:

no known conversion from 'std::__1::vector<bool, std::__1::allocator<bool> >::const_reference' (aka '__bit_const_reference<std::__1::vector<bool, std::__1::allocator<bool> > >')

Guess I'll have to add it after all.

@falbrechtskirchinger falbrechtskirchinger force-pushed the fix-bool-ref branch 2 times, most recently from 9bcdfb2 to 3dac72e Compare August 6, 2022 19:12
@falbrechtskirchinger
Copy link
Contributor Author

Let me walk you through the enable_if constraint:
Since we already have a to_json function for BasicJsonType::boolean_t, we need to disable the vector<bool>::reference/vector<bool>::const_reference overload, if:

  • vector<bool>::reference is the same as BasicJsonType::boolean_t &, or
  • vector<bool>::const_reference, stripped of cvref qualifiers, is the same as BasicJsonType::boolean_t.

This covers all tested and reported STLs.

Copy link
Contributor Author

@falbrechtskirchinger falbrechtskirchinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Copy link
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@nlohmann nlohmann added this to the Release 3.11.2 milestone Aug 7, 2022
@nlohmann nlohmann merged commit f1e3407 into nlohmann:develop Aug 7, 2022
@falbrechtskirchinger falbrechtskirchinger deleted the fix-bool-ref branch August 7, 2022 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants