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

better subview #709

Closed
wants to merge 4 commits into from
Closed

better subview #709

wants to merge 4 commits into from

Conversation

vinniefalco
Copy link
Member

fix #708

@codecov
Copy link

codecov bot commented May 26, 2022

Codecov Report

Merging #709 (43e50e7) into develop (c4ab4dd) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #709   +/-   ##
========================================
  Coverage    99.08%   99.08%           
========================================
  Files           69       69           
  Lines         6547     6553    +6     
========================================
+ Hits          6487     6493    +6     
  Misses          60       60           
Impacted Files Coverage Δ
include/boost/json/object.hpp 100.00% <ø> (ø)
include/boost/json/string.hpp 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c4ab4dd...43e50e7. Read the comment docs.

@cppalliance-bot
Copy link

@Lastique
Copy link
Member

Comparison operators need to change as well.

@Lastique
Copy link
Member

Also, although it might fix the Boost.JSON building issue, users' code that invokes conversion to boost::core::string_view will still remain affected. I think, the conversion itself needs to be fixed (i.e. one of the two conversion paths should be made less preferable).

@vinniefalco
Copy link
Member Author

Maybe we should apply a gcc 8.3 specific fix?

@vinniefalco
Copy link
Member Author

I updated the comparisons

@@ -2782,7 +2792,7 @@ typename std::enable_if<
operator==(T const& lhs, U const& rhs) noexcept
#endif
{
return string_view(lhs) == string_view(rhs);
return lhs.subview() == rhs.subview();
Copy link
Member

Choose a reason for hiding this comment

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

This may not work if either T or U is neither string or string_view. You're testing for is_convertible, which allows any type that may not have subview() member.

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm this requires a bit more thought, thanks

@Lastique
Copy link
Member

Maybe we should apply a gcc 8.3 specific fix?

I'm not opposed to a gcc-specific workaround. I've just not figured out a way to make the conversions to compile.

@vinniefalco
Copy link
Member Author

How about this? 1949b0a

@vinniefalco
Copy link
Member Author

It seems to compile on gcc 8.3 but I am not sure whether I need to actually instantiate those operators or not
https://godbolt.org/z/EdxzEGT4v

@Lastique
Copy link
Member

Lastique commented May 26, 2022

Seems to be working for my local build of Boost.JSON. Also on godbolt. Thanks!

PS: mc1 case in the godbolt link above doesn't work, but I don't think it did before. Not sure if this is intended, but it's good enough for me.

@cppalliance-bot
Copy link

@cppalliance-bot
Copy link

@cppalliance-bot
Copy link

@cppalliance-bot
Copy link

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.

1.79: Build error with gcc 8 in C++17 mode: ambiguous conversion to string_view
4 participants