Re: [Boost-users] Boost range changes [1.37.0]

Subject: Re: [Boost-users] Boost range changes [1.37.0]
From: Dave Handley (dave_at_[hidden])
Date: 2008-11-19 22:47:49

Please see comments inline:

"Tomas Puverle" <Tomas.Puverle_at_[hidden]> wrote in message news:loom.20081120T001601-724_at_post.gmane.org...
> Thorsten,
> 
> Thanks for the reply.  See comments inline.
> 
>> The change has happened long ago. It was a design mistake to try to make 
>> a range of singular iterators valid since it adds overhead also for 
>> those that don't need it.
> 
> When you say "overhead", are you speaking of size or speed?  
> Was this overhead demonstrated by some particular use case?  
> Does this mean you may be also removing the "singular" flag at some point?
> Is there a supported way of constructing valid empty ranges?  I can't see that
> there's a way to do it.

I'm also not sure about the overhead issue here.  Checking a single bool inside size and/or empty strikes me as a trivial matter - no more complex than many things in the standard library or elsewhere in boost.

<snip>
> 
> IMHO, this is obfuscated.  There is a difference between an empty range and a
> range which is optional.  In line with the std containers, I would argue that a
> default constructed range should be empty.

I have to agree with this premise.  In line with a lot of proxy or container type objects I would expect a default constructed object to do something sensible.  I wouldn't expect, for example, a boost.shared_ptr to default construct to an object that asserts when you check it for NULL.  This change appears to be quite a severe change in functionality.

<snip>
> In any case, I've enjoyed using your library.  I found it so useful I've built
> four other libraries, which are mostly based on passing data around through
> Boost.Ranges.  I just wish that all of my code wasn't broken now that 
> I've upgraded.
> 

This is a point on which I feel I have to amplify.  I've been using Boost for quite a long time now, and been through numerous upgrades, many of which I've managed the upgrade at work.  Breaking changes are always the thing that hurts the most during an upgrade.  Especially now that boost has gone to an accelerated release cycle where we see such regular new releases.  I'm sure that many users of Boost like myself don't get to upgrade to every release, so moving to a new release involves jumping through 2 or 3 releases.  Breaking changes in this context are very painful.  As a library writer myself, where I am forced to do a breaking change, I always try and make it something that will break on compile - otherwise my users cannot be confident in doing a library upgrade without a very extensive testing schedule - quite often this results in users simply not upgrading.

With this particular change, I feel even more strongly.  Assertions in library code can often be a bad thing.  If the users of the library are not explicitly aware of a change that takes out a check and replaces it with an assertion, then you get applications which can either abort unexpectedly (if not built with NDEBUG) or worse still just do something different in a silent fail.  This is just downright dangerous. Personally, I would strongly support backing out of a change like this.  However, if Thorsten strongly disagrees, I would at least recommend as a compromise position to introduce a define along the lines of BOOST_NO_ASSERT (or maybe BOOST_RANGE_NO_ASSERT) that gets rid of the assertion and does something sensible instead.  The only times I like to see assertions in libraries are: firstly, states which should be impossible to get into and are therefore testing for either corruption or a problem in the library; or secondly, occasions where you are going to crash anyway if you continue - for example if after the assertion you are about to dereference a NULL or uninitialised pointer.  Things which are common states that systems using the library could get into should do something sensible - at worst throw an exception, and at best return a sensible answer.

Please reconsider this change - I have yet to upgrade to 1.33.7 yet, and knowing this type of change is present there makes me highly reluctant to go ahead with an upgrade.  I'm not sure (just covering the post from Steven) that going through an entire codebase and changing calls to empty with calls to a new template function "is_empty_or_singular" is really a palatable solution.  Boost provides a fantastically useful set of libraries - they have certainly significantly changed the way I code in C++; but breaking changes should always be very carefully thought about, they have a nasty habit of leaving people isolated in old versions of boost, and also have a habit of driving people away from using boost.

Regards

Dave