[utility] prior(it, n) for n being an unsigned type
Hi! This is a re-post from boost-users mailing list, as I didn't get any response there. I've just finished investigating an issue in my code which led it to an assert/abort in certain cases. It turned out it was caused by the use of boost::prior(it, n) where n was unsigned int with some non-zero value. I found a question [1] on the mailing list back in 2004 which describes this issue. Additionally a solution to this problem was also posted in one of the replies [2]. My question is - why hasn't it made into the repository yet? The current behavior of boost::prior(it, n) when n is unsigned type is far from the one expected by its potential users so I think it should either be properly documented (warned) or fixed immediately. Am I not seeing something? WBR, Adam Romanek [1] http://lists.boost.org/Archives/boost/2004/02/61556.php [2] http://lists.boost.org/Archives/boost/2004/02/61578.php
On Monday 23 June 2014 08:40:29 Adam Romanek wrote:
Hi!
This is a re-post from boost-users mailing list, as I didn't get any response there.
I've just finished investigating an issue in my code which led it to an assert/abort in certain cases. It turned out it was caused by the use of boost::prior(it, n) where n was unsigned int with some non-zero value.
I found a question [1] on the mailing list back in 2004 which describes this issue. Additionally a solution to this problem was also posted in one of the replies [2].
My question is - why hasn't it made into the repository yet?
Probably because it got forgotten.
The current behavior of boost::prior(it, n) when n is unsigned type is far from the one expected by its potential users so I think it should either be properly documented (warned) or fixed immediately.
I agree this should be fixed. Although it would probably be better to follow the standard interface: template <class BidirectionalIterator> BidirectionalIterator prev(BidirectionalIterator x, typename std::iterator_traits<BidirectionalIterator>::difference_type n = 1); I.e. the second argument should be accepted as a signed value and not as a template parameter. I'll apply the change later if noone objects.
On Monday 23 June 2014 12:29:27 you wrote:
On Monday 23 June 2014 08:40:29 Adam Romanek wrote:
Hi!
This is a re-post from boost-users mailing list, as I didn't get any response there.
I've just finished investigating an issue in my code which led it to an assert/abort in certain cases. It turned out it was caused by the use of boost::prior(it, n) where n was unsigned int with some non-zero value.
I found a question [1] on the mailing list back in 2004 which describes this issue. Additionally a solution to this problem was also posted in one of the replies [2].
My question is - why hasn't it made into the repository yet?
Probably because it got forgotten.
The current behavior of boost::prior(it, n) when n is unsigned type is far from the one expected by its potential users so I think it should either be properly documented (warned) or fixed immediately.
I agree this should be fixed. Although it would probably be better to follow the standard interface:
template <class BidirectionalIterator> BidirectionalIterator prev(BidirectionalIterator x, typename std::iterator_traits<BidirectionalIterator>::difference_type n = 1);
I.e. the second argument should be accepted as a signed value and not as a template parameter.
I'll apply the change later if noone objects.
I've created a pull request: https://github.com/boostorg/utility/pull/15 After I read that old discussion I decided to go with a slightly more complicated solution than I originally intended.
On 23.06.2014 23:21, Andrey Semashev wrote:
(...)
I've created a pull request:
https://github.com/boostorg/utility/pull/15
After I read that old discussion I decided to go with a slightly more complicated solution than I originally intended.
It became a bit complicated, for such a "simple" utility ;) But I'm glad it's been fixed. Thanks! Just out of curiosity - why haven't you chosen the std::reverse_iterator -based approach described in the old thread? WBR, Adam Romanek
On 24/06/2014 18:21, Adam Romanek wrote:
Just out of curiosity - why haven't you chosen the std::reverse_iterator -based approach described in the old thread?
"It should also work with integers now, as was originally intended by David Abrahams" + BOOST_REQUIRE(boost::next(5) == 6); + BOOST_REQUIRE(boost::next(5, 7) == 12);
On Tue, Jun 24, 2014 at 10:21 AM, Adam Romanek <romanek.adam@gmail.com> wrote:
On 23.06.2014 23:21, Andrey Semashev wrote:
(...)
I've created a pull request:
https://github.com/boostorg/utility/pull/15
After I read that old discussion I decided to go with a slightly more complicated solution than I originally intended.
It became a bit complicated, for such a "simple" utility ;) But I'm glad it's been fixed. Thanks!
Not yet. I'll apply the pull request if noone objects in a few days.
Just out of curiosity - why haven't you chosen the std::reverse_iterator -based approach described in the old thread?
Dave Abrahams (the original author) stated that he intended this utility to be also usable with integers. You can see that in the discussion [1]. I felt obligated to fulfill that requirement, and using std::advance and reverse_iterator contradicted that. I could have used reverse_iterator in the iterator-related part but, well, deducing the signed integer type looked more straightforward to me. [1] http://lists.boost.org/Archives/boost/2004/02/61583.php
On 24.06.2014 08:30, Andrey Semashev wrote:
On Tue, Jun 24, 2014 at 10:21 AM, Adam Romanek <romanek.adam@gmail.com> wrote:
On 23.06.2014 23:21, Andrey Semashev wrote:
(...)
I've created a pull request:
https://github.com/boostorg/utility/pull/15
After I read that old discussion I decided to go with a slightly more complicated solution than I originally intended.
It became a bit complicated, for such a "simple" utility ;) But I'm glad it's been fixed. Thanks!
Not yet. I'll apply the pull request if noone objects in a few days.
I meant I'm glad there's some movement here after all these years. Hope to see it merged.
Just out of curiosity - why haven't you chosen the std::reverse_iterator -based approach described in the old thread?
Dave Abrahams (the original author) stated that he intended this utility to be also usable with integers. You can see that in the discussion [1]. I felt obligated to fulfill that requirement, and using std::advance and reverse_iterator contradicted that. I could have used reverse_iterator in the iterator-related part but, well, deducing the signed integer type looked more straightforward to me.
Oh, I somehow forgot about that. Thanks for pointing this out. WBR, Adam Romanek
On 24.06.2014 08:30, Andrey Semashev wrote:
On Tue, Jun 24, 2014 at 10:21 AM, Adam Romanek <romanek.adam@gmail.com> wrote:
On 23.06.2014 23:21, Andrey Semashev wrote:
(...)
I've created a pull request:
Hi Andrey, I can see the change has been merged into the 'develop' branch. What are the chances it would be merged to the 'master' branch and released as part of Boost 1.56? Moreover, I think it would be nice if this bug-fix was mentioned in the release notes, right? Do you need a ticket for the original issue as to be able to create a link in the release note? WBR, Adam Romanek
On Monday 30 June 2014 12:31:36 Adam Romanek wrote:
Hi Andrey,
I can see the change has been merged into the 'develop' branch. What are the chances it would be merged to the 'master' branch and released as part of Boost 1.56?
It broke some tests, see my recent post [1]. This change won't be merged to master until these problems are resolved. Strictly speaking, the master branch of the superproject should be long frozen by now, so this change missed the official window for 1.56. However I can see that it is still automatically updated, so the release is probably delayed again. Not sure if this means the window is extended.
Moreover, I think it would be nice if this bug-fix was mentioned in the release notes, right? Do you need a ticket for the original issue as to be able to create a link in the release note?
The ticket is not required to put a line to release notes. If one already exists I can reference it, of course. [1] [iterator] Make operators conditionally defined.
On 30.06.2014 12:46, Andrey Semashev wrote:
On Monday 30 June 2014 12:31:36 Adam Romanek wrote:
Hi Andrey,
I can see the change has been merged into the 'develop' branch. What are the chances it would be merged to the 'master' branch and released as part of Boost 1.56?
It broke some tests, see my recent post [1]. This change won't be merged to master until these problems are resolved.
The problem with the tests suggests that this change may also cause some regression in end-user code, but I guess there's nothing we can do about it (except for putting a few words in the release notes).
Strictly speaking, the master branch of the superproject should be long frozen by now, so this change missed the official window for 1.56. However I can see that it is still automatically updated, so the release is probably delayed again. Not sure if this means the window is extended.
Moreover, I think it would be nice if this bug-fix was mentioned in the release notes, right? Do you need a ticket for the original issue as to be able to create a link in the release note?
The ticket is not required to put a line to release notes. If one already exists I can reference it, of course.
I'm not aware of such a ticket. Thanks for making this clear. WBR, Adam Romanek
Hi Adam, Am Montag, 30. Juni 2014, 13:19:33 schrieb Adam Romanek:
It broke some tests, see my recent post [1]. This change won't be merged to master until these problems are resolved.
The problem with the tests suggests that this change may also cause some regression in end-user code, but I guess there's nothing we can do about it (except for putting a few words in the release notes).
This definitely broke some code here. Some more words on how to adapt to the changes will be appreciated. Or even a non-breaking fix. Yours, Jürgen -- * Dipl.-Math. Jürgen Hunold ! * voice: ++49 4257 300 ! Fährstraße 1 * fax : ++49 4257 300 ! 31609 Balge/Sebbenhausen * jhunold@gmx.eu ! Germany
On Monday 30 June 2014 15:12:37 Jürgen Hunold wrote:
Hi Adam,
Am Montag, 30. Juni 2014, 13:19:33 schrieb Adam Romanek:
It broke some tests, see my recent post [1]. This change won't be merged to master until these problems are resolved.
The problem with the tests suggests that this change may also cause some regression in end-user code, but I guess there's nothing we can do about it (except for putting a few words in the release notes).
This definitely broke some code here. Some more words on how to adapt to the changes will be appreciated. Or even a non-breaking fix.
The functions now detect if the first operand supports arithmetic operators (+=, + -=, -) and uses them if they are defined. The related pull request for Boost.Iterator makes iterator_facade to not define these operators if the iterator doesn't support them. If your case does not involve Boost.Iterator iterators then you need to make a similar change to your iterators that you use with next()/prior().
participants (4)
-
Adam Romanek -
Andrey Semashev -
Gavin Lambert -
Jürgen Hunold