On Sat, Nov 22, 2008 at 4:25 PM, Mathias Gaunard < mathias.gaunard@ens-lyon.org> wrote:
Tomas Puverle wrote:
Here are the reasons why I think Boost.Range is broken:
1) Empty ranges are useless and they cannot even be reliably tested for emptiness. The empty() function now asserts and the is_singular() function behaves differently between debug and release builds, making it impossible to detect if a range is, in fact, empty. In addition, this is not documented well and can lead to subtle bugs and undefined behaviour, which will only manifest itself in release builds. 2) The behaviour is unintuitive. Range is a generalisation of the interface of the std::containers. With this change, containers and ranges can no longer be used in the same code path. 3) Reintroducing the "singular" member into release builds to make the is_singular() function work correctly will defeat the purpose of the size optimisation, while still not achieving interface compatibility with std::containers. 4) Having the additional if() condition in size() and empty()is unlikely to be a large burden on most programs. I would expect most programs will spend more time iterating data than testing ranges for emptiness. 5) The change could be reverted without affecting users. For those who are relying on the new behaviour, the change to empty() and size() should be immaterial, as they cannot be calling them now on singular ranges anyway.
This is a bad explanation which can prove misleading.
The reason why you think Boost.Range is broken is because iterator_range (which is nothing more than a fancy std::pair, I never found the use of it myself) used to allow operations such as "empty" on an uninitialized range but doesn't anymore.
With such a notion this, indeed, is a rather useless component. I have to agree with Tomas and others in one of their points: one of the major advantages or ranges before pairs of iterators is that they attempt to reproduce containers as far as possible. Yes, in general default-constructed iterators are singular and they cannot be used reliably. However, not all iterators behave that way. Pointers, for example, are NULL on default-initialization (which is used in the default constructor of the iterator_range class). I can well have my own iterators that are default-constructible and allow comparison in this state. And I don't understand why an iterator range of these iterators suddenly restricts me from being able to compare these iterators in empty(), for example. The documentation of the iterator_range states for several functions, such as size() and operator[], that these functions depend on the iterator nature. I believe, empty() could just do the same - rely on the ability to compare iterators, even default-initialized ones. Of course, this should be clearly stated in the docs. If it was just me, default-constructing iterators and ranges shouldn't even
be allowed (unless that is sufficient that initialize it). A range that is not properly initialized is not in any usable state. It's not an empty range, it's more like a pointer to somewhere random. Just don't try to use it. If Boost.Range used to allow it, that was a bug; good thing it is now fixed.
As for is_singular, that function shouldn't be exposed to the public.
I agree that is_singular is evil. More over, I am sure that asserts based on it are also evil. My point is: don't try to make iterator_range more clever than it is stated in the docs (that is, a pair of iterators with a forwarding interface of a container).