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

remove_prefix and remove_suffix overly permissive #92

Closed
sehe opened this issue May 2, 2022 · 7 comments · Fixed by #98
Closed

remove_prefix and remove_suffix overly permissive #92

sehe opened this issue May 2, 2022 · 7 comments · Fixed by #98

Comments

@sehe
Copy link

sehe commented May 2, 2022

The implementation contains extra logic:

BOOST_CXX14_CONSTEXPR void remove_prefix(size_type n) {
   if ( n > len_ )
       n = len_;

While the extra check for out-of-range argument value seems nice, it can hide problems. Worse, when eventually users will (have to) switch to other implementations, this will silently introduce UB into code that was unwittingly/wittingly relying on the non-standard behaviour.

Suggest to replace the condition with a BOOST_ASSERT so the danger is removed.

Lastique added a commit that referenced this issue May 2, 2022
This is in line with std::string_view::remove_prefix/suffix definition.

Closes #92.
@mclow
Copy link
Contributor

mclow commented May 30, 2022

As I read your issue, I am thinking "you want to count on undefined behavior?" - because that's what you're asking for.

@mclow
Copy link
Contributor

mclow commented May 30, 2022

Maybe a better comment is "You want consistent undefined behavior?"

@Lastique
Copy link
Member

I think the incentive is to be able to detect incorrect arguments (in debug builds) rather than silently accept them. In release builds let the UB take place.

@sehe
Copy link
Author

sehe commented May 30, 2022

All in all, you got it. Let me reinforce re:

Maybe a better comment is "You want consistent undefined behavior?"

Yes. Because I want to avoid the silent pitfall where people rely on this lenient behavior of a supposed standard-compliant string_view only to get spurious UB when they finally do move to std::string_view (or some other implementation).

Hyrum's Law with a twist

@mclow
Copy link
Contributor

mclow commented May 30, 2022

There is no such thing as "consistent undefined behavior". That's a fallacy. If you want implementations to behave identically, then the behavior is defined.

@sehe
Copy link
Author

sehe commented May 30, 2022

@mclow It was you planting the fallacy, and I happily obliged because it's not about that. I rather took it to mean you got what it is about.

Some approaches open up the user to surprise, others do not. We can choose. If¹ you don't care, fine. I do.

Meanwhile, Boost Utility doesn't document these functions at all (beyond listing their existence). People will go and check the implementation to see what guarantees are made.

Going back to your initial comment ("I am thinking "you want to count on undefined behavior?" - because that's what you're asking for.") can you see I'm merely trying to be realistic?

¹ note the if

@vinniefalco
Copy link
Member

This whole debate is pointless, just look at Peter's implementation and see what he did if you want to know what is correct: https://github.com/boostorg/core/blob/ac9d79992e0f5bcdb8953330c67fe9c706c9f79f/include/boost/core/detail/string_view.hpp#L532

seems like he doesn't perform the check.

Lastique added a commit that referenced this issue Jul 5, 2022
…ons.

This is in line with std::string_view::remove_prefix/suffix definition, where
calling the method with n > size() is UB. We're keeping the check to clamp
n to size() for now for backward compatibility so that it can be eventually
removed.

Closes #92.
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 a pull request may close this issue.

4 participants