boostorg/utility

remove_prefix and remove_suffix overly permissive

Closed this issue · 7 comments

sehe commented

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.

mclow commented

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

mclow commented

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

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 commented

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 commented

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 commented

@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

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.