Let's fix the logical constness of value_initialized!

[niels_dekker]

The data() member function and the conversion operator of boost::value_initialized<T> break the logical constness: both functions are declared "const", while they return a non-const reference to the T object that is owned by value_initialized<T>. I think both of them should be replaced by a pair of overloaded functions for const and non-const access.

I don't think that there ever was a good reason for the data() function to break the logical constness. But clearly, there was a reason for the conversion operator to do so, at the time value_initialized was first released, in 2002. At that time, some commonly used compilers would complain about ambiguity when the conversion operator would be overloaded for const and non-const, in some specific use cases. As described at http://www.boost.org/doc/libs/1_37_0/libs/utility/value_init.htm#val_init

In January 2004, "deep const semantics" still wasn't feasible, apparently due to compiler errors in VC++ and Borland: [boost] value_initialized, deep const, and template instantiation ​http://lists.boost.org/Archives/boost/2004/01/59987.php ​http://lists.boost.org/Archives/boost/2004/01/60020.php

Nowadays I think that things have changed for the better. I just replaced the single conversion operator by overloads for const and non-const in my local copy, and tested for VC++ 2003 (7.1), VC++ 2008 (9.0), and CodeGear 2009 (6.10). And all of them passed the tests!

Therefore I'd like to propose fixing the issue, according to the attached patch. Fernando, shall we give it a try?

I'm aware that the proposed fix potentially breaks some user code, but I think that those cases are easily fixed, and I think that most Boost users are likely to appreciate logical constness.

[niels_dekker]

Fernando Cacciola allowed me to fix the issue :-) See also "[boost] [utility] A fix for the const issue of value_initialized", ​http://lists.boost.org/Archives/boost/2009/02/148489.php

[niels_dekker]

Committed to the trunk, two days ago: changesets [51355] and [51356]

So far, I haven't seen any regression failure because of these commits, but I'll keep an eye at http://www.boost.org/development/tests/trunk/developer/utility_.html

[niels_dekker]

Hmmm... My commits ([51355] and [51356]) have caused a compile time regression failure of Intel Version 9.0 Build 20060222Z (see below).

I still have to figure out how to work around this issue. Hopefully the commits don't need to be reverted. Any help is appreciated!

http://www.boost.org/development/tests/trunk/output/Sandia-intel-9-0-boost-bin-v2-libs-utility-test-value_init_test-test-intel-linux-9-0-debug.html says: Compile [2009-02-24 12:19:24 UTC]: fail

"/usr/local/intel/cc/9.0-032/bin/icpc" -c -xc++ -O0 -g -Ob0 -w1 -fPIC -DBOOST_ALL_NO_LIB=1 -I".." -c -o "/var/scratch/boost/results/boost/bin.v2/libs/utility/test/value_init_test.test/intel-linux-9.0/debug/value_init_test.o" "../libs/utility/test/../value_init_test.cpp"

../libs/utility/test/../value_init_test.cpp(258): error: more than one operator "==" matches these operands:

built-in operator "pointer == pointer" built-in operator "pointer == pointer" operand types are: boost::value_initialized<NonPOD *> == NonPOD *const

BOOST_CHECK ( x == z ) ;

detected during instantiation of "bool test(const T &, const T &) [with T=NonPOD *]"

../libs/utility/test/../value_init_test.cpp(265): error: more than one operator "==" matches these operands:

built-in operator "pointer == pointer" built-in operator "pointer == pointer" operand types are: const boost::value_initialized<NonPOD *> == NonPOD *const

BOOST_CHECK ( x_c == z ) ;

detected during instantiation of "bool test(const T &, const T &) [with T=NonPOD *]"

compilation aborted for ../libs/utility/test/../value_init_test.cpp (code 2)

[Daniel James]

This should either be fixed or reverted. If it's a bug with Intel 9 then it would probably be best to use a BOOST_WORKAROUND macro for the compilers which have this bug. And you'll need to update the version number in the documentation, since 1.39 has the old version.

[niels_dekker]

Daniel, thanks for your feedback. Yes, I would still like the issue to be fixed. It's now more than four months after committing the fix to the trunk, [51355], and I haven't heard any compaint. (I did announce the fix at the Boost dev mailing list.) Intel 9 is the only compiler on the regression site that doesn't compile implicit conversions from value_initialized<T> to reference-to-T anymore, because of the fix. It's no problem to Intel 10 and 11, and any other compiler included with the regression tests.

There are various possible workarounds for Intel 9:

1. Do nothing. Intel 9 users can still retrieve a reference to the value_initialized data by using boost::get, or value_initialized::data()
2. Revert the fix, but only for Intel 9 (using BOOST_WORKAROUND). Leave the fix in there for other compilers.
3. Offer to Intel 9 users only the const-version of the conversion operator (using BOOST_WORKAROUND)
4. Offer to Intel 9 users only the non-const-version of the conversion operator (using BOOST_WORKAROUND)

I do prefer workaround "d.", which allows Intel 9 users to convert from value_initialized<T> to T&, but protects const value_initialized<T> objects against such a conversion. As attached: intel_9_value_init_conversion-operator.patch Please let me know if you think this patch is fine to you.

PS Thanks also for reminding me to update the version number in the documentation!

[Daniel James]

I'm really not the person to ask, I'm mainly trying to get some of the 'core' libraries to be consistent on trunk and release in response to some recent mails on the development list.

My normal preference would be to preserve the old behaviour for broken compilers (option b, I think) but the old behaviour looks wrong. I don't know why it was written that way so I don't feel qualified to express a preference. So I'm fine with your patch. No one else seems to care so I would suggest that you go ahead with what you think best. You've already asked for feedback on the development list, so I don't think you have to do it again, unless you feel it's necessary.

Thanks for sorting this out.

[niels_dekker]

You're welcome, Daniel.

I have a problem with "option b", as it would make the behaviour of value_initialized compiler-dependent. So I sticked with "option d", by applying intel_9_value_init_conversion-operator.patch: changeset [54502]. Hopefully it will lower the number of Intel 9 compile errors from 2 to 1, as the compiler should now be able to accept BOOST_CHECK(x == z). If so, I'll mark the other Intel 9 compile error as an "expected failure", and do a merge to the release branch, when the time is right.

PS I also updated the documentation, as you suggested: changeset [54503]

[niels_dekker]

So I marked the Intel 9 failure: changeset [54560]

Unfortunately the number of Intel 9 compile errors is not reduced by the workaround I added with changeset [54502]. Instead, it has gone up from 2 to 33! Before committing the workaround, Intel 9 apparently only failed to convert from value_initialized<T> to T when T was a pointer. While the workaround disables conversion from any const value_initialized<T>, for Intel 9. But it does enable conversion from any non-const value_initialized<T>, including when T is a pointer. So there's clearly a trade-off here.

Sandia-intel-9-0-boost-bin-v2-libs-utility-test-value_init_test-test-intel-linux-9-0-debug.html now reports:

Test output: Sandia-intel-9.0 - utility - value_init_test / intel-linux-9.0

Rev 54533 / Tue, 30 Jun 2009 10:33:23 +0000 Report Time: Tue, 30 Jun 2009 20:15:05 +0000

Compile [2009-06-30 12:11:26 UTC]: fail

"/usr/local/intel/cc/9.0-032/bin/icpc" -c -xc++ -O0 -g -w1 -Ob0 -fPIC -DBOOST_ALL_NO_LIB=1 -I".." -c -o "/var/scratch/boost/results/boost/bin.v2/libs/utility/test/value_init_test.test/intel-linux-9.0/debug/value_init_test.o" "../libs/utility/test/../value_init_test.cpp"

../libs/utility/test/../value_init_test.cpp(261): error: no operator "==" matches these operands

operand types are: const int == const boost::value_initialized<int>

BOOST_CHECK ( y == x_c ) ;

detected during instantiation of "bool test(const T &, const T &) [with T=int]"

../libs/utility/test/../value_init_test.cpp(265): error: no operator "==" matches these operands

operand types are: const boost::value_initialized<int> == const int

BOOST_CHECK ( x_c == z ) ;

detected during instantiation of "bool test(const T &, const T &) [with T=int]"

[ snip ]

../libs/utility/test/../value_init_test.cpp(283): error: no operator "==" matches these operands

operand types are: const AggregatePODStructWrapper == const boost::value_initialized<const AggregatePODStructWrapper>

BOOST_CHECK ( y == cx_c ) ;

detected during instantiation of "bool test(const T &, const T &) [with T=AggregatePODStructWrapper]"

compilation aborted for ../libs/utility/test/../value_init_test.cpp (code 2)

[niels_dekker]

I just asked for feedback from Boost users: [Boost-users] [utility] Anyone here using Intel 9.0 compiler + value_initialized?, ​http://lists.boost.org/boost-users/2009/07/49498.php

[niels_dekker]

Fernando Cacciola mailed me that it seems okay to him to just leave implicit conversion from value_initialized<T*> broken for Intel 9.0, instead of having it patched. Which also makes sense to me because it appears a rather rare use case, and it's an old compiler version anyway. So I committed changeset [54832], which reverted intel_9_value_init_conversion-operator.patch

[niels_dekker]

FYI, Yesterday I merged the fix from the trunk to the release branch: changeset [56547]. Related merge of value_init_test: Changeset [56543]. I also informed people at the [boost] mailing list: "[boost] [utility] Fix for const issue of value_initialized merged to release", ​http://lists.boost.org/Archives/boost/2009/10/156798.php

Looks like the fix will be included with Boost 1.41.0 :-)