[V1.46] [Iterator] Ask for release notes modification
Hi, I have some comments on the Boost 1.46 release notes for Boost.Iterator. (http://svn.boost.org/svn/boost/website/public_html/beta/feed/history/boost_1...) Here are the release notes for Boost.Iterator: * fixed problem with `implicit_cast` ([@https://svn.boost.org/trac/boost/ticket/3645 #3645]) * added `function_input_iterator` ([@https://svn.boost.org/trac/boost/ticket/2893 #2893]) * `transform_iterator` now uses `boost::result_of` to determine functor result type ([@https://svn.boost.org/trac/boost/ticket/1427 #1427]) The first item seems OK :) The second item needs to be deleted, since `function_input_iterator` is not merged into release branch. The third item also needs to be changed. As the release notes say, `transform_iterator` now uses `boost::result_of` to determine functor result type (in prior versions, it uses `result_type`). But there is a bug: it should use `boost::result_of<Iterator::reference>` as the documentation says, but it uses `boost::result_of<Iterator::value_type>`. (This was fixed in trunk a week ago, but it was too late to merged to release.) This bug makes subtle errors and confuses the users. Is it better to mention the bug in the release notes explicitly? Something like: * BUG: `transform_iterator` uses `boost::result_of<Iterator::value_type>` to determine functor result type instead of `boost::result_of<Iterator::reference>` ([@https://svn.boost.org/trac/boost/ticket/1427 #1427]) Or simply delete this item? Regards, Michel
On Tue, Jan 25, 2011 at 5:21 AM, Michel MORIN <mimomorin@gmail.com> wrote:
Hi,
I have some comments on the Boost 1.46 release notes for Boost.Iterator. (http://svn.boost.org/svn/boost/website/public_html/beta/feed/history/boost_1...)
Here are the release notes for Boost.Iterator:
* fixed problem with `implicit_cast` ([@https://svn.boost.org/trac/boost/ticket/3645 #3645]) * added `function_input_iterator` ([@https://svn.boost.org/trac/boost/ticket/2893 #2893]) * `transform_iterator` now uses `boost::result_of` to determine functor result type ([@https://svn.boost.org/trac/boost/ticket/1427 #1427])
The first item seems OK :)
The second item needs to be deleted, since `function_input_iterator` is not merged into release branch.
Should it be merged? --Beman
On Jan 25, 2011, at 4:49 AM, Beman Dawes wrote:
On Tue, Jan 25, 2011 at 5:21 AM, Michel MORIN <mimomorin@gmail.com> wrote:
Hi,
I have some comments on the Boost 1.46 release notes for Boost.Iterator. (http://svn.boost.org/svn/boost/website/public_html/beta/feed/history/boost_1...)
Here are the release notes for Boost.Iterator:
* fixed problem with `implicit_cast` ([@https://svn.boost.org/trac/boost/ticket/3645 #3645]) * added `function_input_iterator` ([@https://svn.boost.org/trac/boost/ticket/2893 #2893]) * `transform_iterator` now uses `boost::result_of` to determine functor result type ([@https://svn.boost.org/trac/boost/ticket/1427 #1427])
The first item seems OK :)
The second item needs to be deleted, since `function_input_iterator` is not merged into release branch.
I applied this patch to the trunk. The test results look fine - but there really needs to be some tests for it. IMHO, this is a "new feature", rather than a "bug fix", and if I had done the merge two weeks ago, it would be a good thing to have in the release. My bad. Right now, I'm thinking that it's very late in the release cycle to be adding new functionality. -- Marshall Marshall Clow Idio Software <mailto:mclow.lists@gmail.com> A.D. 1517: Martin Luther nails his 95 Theses to the church door and is promptly moderated down to (-1, Flamebait). -- Yu Suzuki
On 25/01/2011 14:51, Marshall Clow wrote:
I applied this patch to the trunk. The test results look fine - but there really needs to be some tests for it.
IMHO, this is a "new feature", rather than a "bug fix", and if I had done the merge two weeks ago, it would be a good thing to have in the release.
My bad.
Right now, I'm thinking that it's very late in the release cycle to be adding new functionality.
I see two alternatives: - either the fact that transform_iterator uses boost::result_of is removed from the release branch - or the release branch is fixed so that the feature is not buggy. Shipping new features that do not work is a bit silly. In this particular case, the fix seems trivial, so why not just merge it on the release branch?
At Wed, 26 Jan 2011 13:06:32 +0100, Mathias Gaunard wrote:
On 25/01/2011 14:51, Marshall Clow wrote:
I applied this patch to the trunk. The test results look fine - but there really needs to be some tests for it.
IMHO, this is a "new feature", rather than a "bug fix", and if I had done the merge two weeks ago, it would be a good thing to have in the release.
My bad.
Right now, I'm thinking that it's very late in the release cycle to be adding new functionality.
I see two alternatives: - either the fact that transform_iterator uses boost::result_of is removed from the release branch - or the release branch is fixed so that the feature is not buggy.
Either one of those is OK with me. -- Dave Abrahams BoostPro Computing http://www.boostpro.com
Dave Abrahams wrote:
I see two alternatives: - either the fact that transform_iterator uses boost::result_of is removed from the release branch - or the release branch is fixed so that the feature is not buggy.
Either one of those is OK with me.
After digging into the transform_iterator's result_of problem further, I'm beginning to doubt the correctness of the documentation. I think that boost::result_of<UnaryFunction(Iterator::reference)> (which is described in the documentation) is incorrect, and the correct one is boost::result_of<const UnaryFunction(Iterator::reference)>. This is because 1. The dereference operator of transform_iterator is a const member function. 2. So, in the dereference operator, UnaryFunction is treated as a const object. Hmm, what should we do? Regards, Michel
On 26/01/2011 18:51, Michel MORIN wrote:
Dave Abrahams wrote:
I see two alternatives: - either the fact that transform_iterator uses boost::result_of is removed from the release branch - or the release branch is fixed so that the feature is not buggy.
Either one of those is OK with me.
After digging into the transform_iterator's result_of problem further, I'm beginning to doubt the correctness of the documentation.
I think that boost::result_of<UnaryFunction(Iterator::reference)> (which is described in the documentation) is incorrect, and the correct one is boost::result_of<const UnaryFunction(Iterator::reference)>.
This is because 1. The dereference operator of transform_iterator is a const member function. 2. So, in the dereference operator, UnaryFunction is treated as a const object.
After actually looking at the issue, I can confirm that is correct. Good thing you spotted that (especially important for the decltype-based implementation of result_of).
Hmm, what should we do?
I think the right thing to do is probably to make it use boost::result_of<const UnaryFunction(typename std::iterator_traits<Iterator>::reference)> on the trunk, fix the doc, check the tests still pass on all expected targets, then merge it to release unless the release manager disagrees. I for one have been waiting for this feature for a long time, so I'd be glad to have it (and have it working!) in a release as soon as possible.
Mathias Gaunard wrote
Hmm, what should we do?
I think the right thing to do is probably to make it use boost::result_of<const UnaryFunction(typename std::iterator_traits<Iterator>::reference)> on the trunk, fix the doc, check the tests still pass on all expected targets, then merge it to release unless the release manager disagrees.
I've created a new ticket for this problem. https://svn.boost.org/trac/boost/ticket/5127 In the ticket, patches for the code, the test and the documents are attached. I ran the test and it succeeded in GCC 4.4, 4.5. 4.6. Regards, Michel
On 26 January 2011 23:56, Mathias Gaunard <mathias.gaunard@ens-lyon.org> wrote:
I think the right thing to do is probably to make it use boost::result_of<const UnaryFunction(typename std::iterator_traits<Iterator>::reference)> on the trunk, fix the doc, check the tests still pass on all expected targets, then merge it to release unless the release manager disagrees.
I think it's too late for that, we're being quite strict about new changes on the release branch. It'd be better to revert the change on the release branch (probably after the beta, see what response John gets on his thread about the pathscale changes) and then do it properly in the next release. I assume this should be fairly easy to revert, it seems orthogonal to other changes. Appropriate changes to the documentation are fine. Daniel
Daniel James wrote:
On 26 January 2011 23:56, Mathias Gaunard <mathias.gaunard@ens-lyon.org> wrote:
I think the right thing to do is probably to make it use boost::result_of<const UnaryFunction(typename std::iterator_traits<Iterator>::reference)> on the trunk, fix the doc, check the tests still pass on all expected targets, then merge it to release unless the release manager disagrees.
I think it's too late for that, we're being quite strict about new changes on the release branch. It'd be better to revert the change on the release branch (probably after the beta, see what response John gets on his thread about the pathscale changes) and then do it properly in the next release.
+1 I'm very happy if the feature will be shipped in Boost 1.46, but it's too late. So, IMHO, the way to go is 1. Revert transform_iterator's result_of support. (And delete the corresponding item in the release notes for Boost 1.46.) 2. Ask the authors whether the transform_iterator's documentation is incorrect. (Dave, could you comment on this problem?) 3. Trunk fixed. (Patches are attached in ticket #5127.) 4. Merged into release branch for Boost 1.47. P.S. I set the milestone of the following ticket to Boost 1.47. #5127 (Documentation of transform_iterator is incorrect) https://svn.boost.org/trac/boost/ticket/5127 Regards, Michel
Michel MORIN wrote:
* BUG: `transform_iterator` uses `boost::result_of<Iterator::value_type>` to determine functor result type instead of `boost::result_of<Iterator::reference>` ([@https://svn.boost.org/trac/boost/ticket/1427 #1427])
Sorry, typo... I meant * BUG: `transform_iterator` uses `boost::result_of<UnaryFunction(Iterator::value_type)>` to determine functor result type instead of `boost::result_of<UnaryFunction(Iterator::reference)>` ([@https://svn.boost.org/trac/boost/ticket/1427 #1427]) Regards, Michel
participants (6)
-
Beman Dawes -
Daniel James -
Dave Abrahams -
Marshall Clow -
Mathias Gaunard -
Michel MORIN