[array] VC++ 7.1 bug and a proposed fix
The following code does not compile with VC++ 7.1 and Boost 1.33.1.
I believe the code should compile,
and that this is a VC++ 7.1 bug.
-----------------------------------------------------
#include
Johan Råde wrote:
If I modify the array class by adding a non-templatized assignment operator, as follows, then the code does compile.
---------------------------------------------------------
namespace boost { template
class array { ... array & operator=(const array & rhs) { std::copy(rhs.begin(), rhs.end(), begin()); return *this; } ... }; } ----------------------------------------------------------
So I propose that this assignment operator is added, as a VC++ 7.1 bug workaround, to the array class.
The problem with that workaround is that the array template would no longer produce aggregate types, and we would lose the ability to use brace initialization: std::tr1::array< int, 4 > x = { 0, 1, 2, 3 }; That is an important motivating example for this library, and results in other compromises such as no user-declared constructors. Note that it has been accepted into standard library TR1 in this version, and the working draught for the next Standard Library. Hpwever, there may be language changes coming that will allow us to use brace-initialization with non-aggregate types. In that case we will definitely take another look at the class and see if we want to support this kind of code. I am also concerned that the patch involves overloading an operator that does not seem to be called anywhere in the example. It is an interesting test case though. i/ does it fail on other compilers? ii/ Is it a very specific failure with pointer-to-member, or the first hint of a more general issue that should be solved?
I have contacted Nicolai Josuttis concerning this, and he told me he is not maintaining the array library anymore.
Yes, I have taken on the maintenance now. Thanks for the report, although I don't have an instant solution :¬( -- AlisdairM
AlisdairM wrote:
Johan Råde wrote:
If I modify the array class by adding a non-templatized assignment operator, as follows, then the code does compile.
---------------------------------------------------------
namespace boost { template
class array { ... array & operator=(const array & rhs) { std::copy(rhs.begin(), rhs.end(), begin()); return *this; } ... }; } ----------------------------------------------------------
So I propose that this assignment operator is added, as a VC++ 7.1 bug workaround, to the array class.
The problem with that workaround is that the array template would no longer produce aggregate types, and we would lose the ability to use brace initialization:
std::tr1::array< int, 4 > x = { 0, 1, 2, 3 };
Are you sure? An aggregate can not have user defined constructors, but I believe user defined assignment operators ar OK. I tested with VC++ 7.1, and there my fix can be combined with aggregate initialization. (If I add a constructor though, then aggregate initialization does not work with VC++ 7.1.) So I think my fix works.
i/ does it fail on other compilers? ii/ Is it a very specific failure with pointer-to-member, or the first hint of a more general issue that should be solved?
I have no idea. The error only seems to pop up in fairly complex situations with templates and pointer-to-members.
Johan Råde wrote:
So I propose that this assignment operator is added, as a VC++ 7.1 bug workaround, to the array class.
The problem with that workaround is that the array template would no longer produce aggregate types, and we would lose the ability to use brace initialization:
std::tr1::array< int, 4 > x = { 0, 1, 2, 3 };
Are you sure? An aggregate can not have user defined constructors, but I believe user defined assignment operators ar OK.
I tested with VC++ 7.1, and there my fix can be combined with aggregate initialization. (If I add a constructor though, then aggregate initialization does not work with VC++ 7.1.)
So I think my fix works.
i/ does it fail on other compilers? ii/ Is it a very specific failure with pointer-to-member, or the first hint of a more general issue that should be solved?
I have no idea. The error only seems to pop up in fairly complex situations with templates and pointer-to-members.
I've checked with VC8 and GCC-3.4 and both accept the code. However, I actually think VC7.1 is correct in rejecting it (although possibly for the wrong reasons!). Boost.Array is not intended to be either copy-constructable or assignable, so instantiating a function returning an array-by-value *should* fail to compile. Or at the very least we're in a very grey area: it all depends on whether the body of the function is instantiated when you take it's address - or just it's signature - that's the sort of thing that's entirely at the whim of the compiler :-) John.
Peter Dimov wrote:
John Maddock wrote:
Boost.Array is not intended to be either copy-constructable or assignable, so instantiating a function returning an array-by-value *should* fail to compile.
What makes you think so?
boost::array has a templatized assignment operator, so it is clearly intended to be assignable. All I need is an ordinary non-templatized assignment operator as well, and then my code compiles with VC++ 7.1.
Johan Råde wrote:
Peter Dimov wrote:
John Maddock wrote:
Boost.Array is not intended to be either copy-constructable or assignable, so instantiating a function returning an array-by-value *should* fail to compile.
What makes you think so?
boost::array has a templatized assignment operator, so it is clearly intended to be assignable.
All I need is an ordinary non-templatized assignment operator as well, and then my code compiles with VC++ 7.1.
So it does! I hadn't realised there was a template assignment operator. In that case it's a really weird error, and I don't see the harm in the fix, especially if it's VC7.1 specific. John.
Peter Dimov wrote:
John Maddock wrote:
Boost.Array is not intended to be either copy-constructable or assignable, so instantiating a function returning an array-by-value *should* fail to compile.
What makes you think so?
Because if a function returns type X by value, then the function return statement must surely use either the copy constructor or the assignment operator of X? Likewise returning a C-array by value is not legal. John.
John Maddock wrote:
Peter Dimov wrote:
John Maddock wrote:
Boost.Array is not intended to be either copy-constructable or assignable, so instantiating a function returning an array-by-value *should* fail to compile.
What makes you think so?
Because if a function returns type X by value, then the function return statement must surely use either the copy constructor or the assignment operator of X?
Likewise returning a C-array by value is not legal.
Sorry. What makes you think that Boost.Array is not intended to be copy-constructable or assignable?
Peter Dimov wrote:
Likewise returning a C-array by value is not legal.
Sorry.
What makes you think that Boost.Array is not intended to be copy-constructable or assignable?
'cos I wasn't paying enough attention that's why :-) It is assignable, just not copy-constructable (or at least not without a core change making arrays copy-constructable). I wasn't expecting copy-construction and assignment to behave differently and it confused me. Happens easily :-) John.
John Maddock wrote:
Peter Dimov wrote:
Likewise returning a C-array by value is not legal.
Sorry.
What makes you think that Boost.Array is not intended to be copy-constructable or assignable?
'cos I wasn't paying enough attention that's why :-)
It is assignable, just not copy-constructable (or at least not without a core change making arrays copy-constructable).
C arrays aren't copy-constructable, true. But Boost.Array is, struct { T data[ N ]; } is, std::tr1::array is: "The conditions for an aggregate (8.5.1) shall be met. Class array relies on the implicitly-declared special member functions (12.1, 12.4, and 12.8) to conform to the container requirements table in 23.1."
On 7/25/06 2:05 PM, "John Maddock"
Peter Dimov wrote:
Likewise returning a C-array by value is not legal.
Sorry.
What makes you think that Boost.Array is not intended to be copy-constructable or assignable?
'cos I wasn't paying enough attention that's why :-)
It is assignable, just not copy-constructable (or at least not without a core change making arrays copy-constructable). I wasn't expecting copy-construction and assignment to behave differently and it confused me. Happens easily :-)
AFAIK, class types with (non-static) array members are _both_ assignable and copy-constructible. See section 12.8 in the 2003 C++ standard, paragraph 8 for the copy constructor and paragraph 13 for the assignment operator. But you MUST use the implicit copying routines, at least for the copy constructor, since arrays do _not_ have explicit copying semantics. This means that generally you must wrap array members in an private struct if you need non-implicit construction, using a private static member function that returns the array's initial value. See what I did with the "my_configuration::hook" class in "rational_test.hpp" for an example. Another part of this thread mentioned assignment operator templates. Please remember that C++ does not consider such templates when determining if an automatically-defined copying assignment operator is needed. Such an operator is always a non-template, even if there's a operator template with a pattern that could match. If automatically-defined copying assignment operator won't do what the assignment operator template would, then you must manually recreate the routine with an explicitly-defined non-template copying assignment operator. (A similar rule exists for the copy constructor, such that it won't consider possibly-matching constructor templates.) -- Daryle Walker Mac, Internet, and Video Game Junkie darylew AT hotmail DOT com
Another part of this thread mentioned assignment operator templates. Please remember that C++ does not consider such templates when determining if an automatically-defined copying assignment operator is needed. Such an operator is always a non-template, even if there's a operator template with a pattern that could match. If automatically-defined copying assignment operator won't do what the assignment operator template would, then you must manually recreate the routine with an explicitly-defined non-template copying assignment operator. (A similar rule exists for the copy constructor, such that it won't consider possibly-matching constructor templates.)
The situation with regard to template assignment operators and template constructors is more subtle than many people think. In certain situations they _will_ be used for copies, even in the presence of implicitly defined default functions. Have a look at (especially the postings from kanze): http://groups.google.com/group/comp.lang.c++.moderated/browse_thread/thread/... Regards, Stephan
Daryle Walker wrote:
It is assignable, just not copy-constructable (or at least not without a core change making arrays copy-constructable). I wasn't expecting copy-construction and assignment to behave differently and it confused me. Happens easily :-)
AFAIK, class types with (non-static) array members are _both_ assignable and copy-constructible. See section 12.8 in the 2003 C++ standard, paragraph 8 for the copy constructor and paragraph 13 for the assignment operator. But you MUST use the implicit copying routines, at least for the copy constructor, since arrays do _not_ have explicit copying semantics. This means that generally you must wrap array members in an private struct if you need non-implicit construction, using a private static member function that returns the array's initial value. See what I did with the "my_configuration::hook" class in "rational_test.hpp" for an example.
Doh! Astonishing how much there is in this language to learn (still!) Thanks for the correction, John.
John Maddock wrote:
Daryle Walker wrote:
It is assignable, just not copy-constructable (or at least not without a core change making arrays copy-constructable). I wasn't expecting copy-construction and assignment to behave differently and it confused me. Happens easily :-) AFAIK, class types with (non-static) array members are _both_ assignable and copy-constructible. See section 12.8 in the 2003 C++ standard, paragraph 8 for the copy constructor and paragraph 13 for the assignment operator. But you MUST use the implicit copying routines, at least for the copy constructor, since arrays do _not_ have explicit copying semantics. This means that generally you must wrap array members in an private struct if you need non-implicit construction, using a private static member function that returns the array's initial value. See what I did with the "my_configuration::hook" class in "rational_test.hpp" for an example.
Doh! Astonishing how much there is in this language to learn (still!)
Thanks for the correction,
John.
I'm not sure I'm quite able to follow this discussion, but what is the conclusion? Can I have my fix? --Johan Råde
AlisdairM wrote:
Johan Råde wrote:
If I modify the array class by adding a non-templatized assignment operator, as follows, then the code does compile.
---------------------------------------------------------
namespace boost { template
class array { ... array & operator=(const array & rhs) { std::copy(rhs.begin(), rhs.end(), begin()); return *this; } ... }; } ----------------------------------------------------------
So I propose that this assignment operator is added, as a VC++ 7.1 bug workaround, to the array class.
The problem with that workaround is that the array template would no longer produce aggregate types, and we would lose the ability to use brace initialization:
I have contacted Nicolai Josuttis concerning this, and he told me he is not maintaining the array library anymore.
Yes, I have taken on the maintenance now. Thanks for the report, although I don't have an instant solution :¬(
Alisdair, Let's finish up this issue. Are you going to add the workaround that I proposed? I believe the discussion in this thread led to the conclusion that my workaround is OK. Aggregates can not have user defined constructors, but they can have user defined assignment operators. --Johan Råde
Johan Råde wrote:
Alisdair,
Let's finish up this issue. Are you going to add the workaround that I proposed?
I believe the discussion in this thread led to the conclusion that my workaround is OK. Aggregates can not have user defined constructors, but they can have user defined assignment operators.
In that case the conclusion was wrong! Aggregates cannot have a user-declared copy-assignment operator. Notice that it is specifically the copy-assignment operator, rather than any other user-declared overloads, including member-templates that might match as a copy-assignment operator (eg, template operator taking non-const reference will match better than implicitly declared copy-assignment) However, what we are talking about appear to be a specific workaround for MSVC7.1. So long as MSVC also have a bug that allows user-declared copy assignment operators in aggregates, I am happy with the workaround. What other platforms require this patch? Should we be checking for MSVC8 as well? I'll also want to update the test suite. Not sure if this is appropriate for 1.34 or if we should hold it back on the mainline. The current release seems stalled, so I am hesitant to mess with anything that might push it back further. OTOH, the patch seems so simple ... -- AlisdairM
AlisdairM wrote:
Johan Råde wrote:
Alisdair,
Let's finish up this issue. Are you going to add the workaround that I proposed?
I believe the discussion in this thread led to the conclusion that my workaround is OK. Aggregates can not have user defined constructors, but they can have user defined assignment operators.
In that case the conclusion was wrong! Aggregates cannot have a user-declared copy-assignment operator. Notice that it is specifically the copy-assignment operator, rather than any other user-declared overloads, including member-templates that might match as a copy-assignment operator (eg, template operator taking non-const reference will match better than implicitly declared copy-assignment)
I think that's wrong (and yes, I've changed my mind several times, mostly thanks to the folks on this list!): "An aggregate is an array or a class (clause 9) with no user-declared constructors (12.1), no private or protected non-static data members (clause 11), no base classes (clause 10), and no virtual functions (10.3)." so assignment ops should be OK. However the change should be restricted to VC7.1 just to be sure.
However, what we are talking about appear to be a specific workaround for MSVC7.1. So long as MSVC also have a bug that allows user-declared copy assignment operators in aggregates, I am happy with the workaround.
What other platforms require this patch? Should we be checking for MSVC8 as well?
I checked the original test case with VC8 and gcc, and they did not need the patch. It really does appear to be VC7.1 specific.
I'll also want to update the test suite.
Yep, make sure there are tests for copy construction and assignment :-)
Not sure if this is appropriate for 1.34 or if we should hold it back on the mainline. The current release seems stalled, so I am hesitant to mess with anything that might push it back further. OTOH, the patch seems so simple ...
Borderline I guess. I don't think it's a critical bug, so maybe cvs HEAD would be the safest option? John.
John Maddock wrote:
AlisdairM wrote:
Not sure if this is appropriate for 1.34 or if we should hold it back on the mainline. The current release seems stalled, so I am hesitant to mess with anything that might push it back further. OTOH, the patch seems so simple ...
Borderline I guess. I don't think it's a critical bug, so maybe cvs HEAD would be the safest option?
I'd be perfectly happy with that. --Johan
John Maddock wrote:
I think that's wrong (and yes, I've changed my mind several times, mostly thanks to the folks on this list!):
"An aggregate is an array or a class (clause 9) with no user-declared constructors (12.1), no private or protected non-static data members (clause 11), no base classes (clause 10), and no virtual functions (10.3)."
so assignment ops should be OK.
However the change should be restricted to VC7.1 just to be sure.
Triple-checking, I believe you are correct. (and I have been through this dance more than once over the last 18 months) It does mean that array would never be a POD, and not be admissible in unions. Those are lesser restrictions though, so I am less worried by them. In principle the proposed workaround should be doing exactly the same as the implicitly declared copy-assignment operator, although it is potentially less efficient - especially in the POD case. The question is - should we make this the default behaviour - banning POD-behaviour and unions; accept these restrictions on the MS compiler only and deem such use non-portable; or specialize on MS only, purely for the pointer-to-member case? -- AlisdairM
AlisdairM wrote:
John Maddock wrote:
However the change should be restricted to VC7.1 just to be sure.
Triple-checking, I believe you are correct. (and I have been through this dance more than once over the last 18 months)
It does mean that array would never be a POD, and not be admissible in unions. Those are lesser restrictions though, so I am less worried by them.
Actually if array ever became a non-POD that would be a serious breaking
change: POD's are initialised in the static initialisation phase and we
really do want:
boost::array
In principle the proposed workaround should be doing exactly the same as the implicitly declared copy-assignment operator, although it is potentially less efficient - especially in the POD case.
The question is - should we make this the default behaviour - banning POD-behaviour and unions; accept these restrictions on the MS compiler only and deem such use non-portable; or specialize on MS only, purely for the pointer-to-member case?
Personally I would add the template-assignment op for BOOST_MSVC < 1400
only, would we gain anything from adding it for other compilers? It opens
up an assignment that wasn't there before, so I guess it means array
John Maddock wrote:
Personally I would add the template-assignment op for BOOST_MSVC < 1400 only, would we gain anything from adding it for other compilers? It opens up an assignment that wasn't there before, so I guess it means array
becomes assignable to array which may or may not be a good idea depending on your POV. John.
array already has a template assignment op.
array
Johan Råde wrote:
John Maddock wrote:
Personally I would add the template-assignment op for BOOST_MSVC < 1400 only, would we gain anything from adding it for other compilers? It opens up an assignment that wasn't there before, so I guess it means array
becomes assignable to array
which may or may not be a good idea depending on your POV. John.
array already has a template assignment op. array
already is assignable to array The question is, should we add a non-template assignment op for VC++ 7.1, as a compiler bug workaround.
Oh, Darn. Sorry for the confusion. That one is problematic as it really would make the array a non-POD so initialisation would occur during the dynamic phase rather than the static phase. IMO that would be a deal braker :-( John.
John Maddock wrote:
Johan Råde wrote:
John Maddock wrote:
Personally I would add the template-assignment op for BOOST_MSVC < 1400 only, would we gain anything from adding it for other compilers? It opens up an assignment that wasn't there before, so I guess it means array
becomes assignable to array
which may or may not be a good idea depending on your POV. John. array already has a template assignment op. array already is assignable to array The question is, should we add a non-template assignment op for VC++ 7.1, as a compiler bug workaround.
Oh, Darn. Sorry for the confusion.
That one is problematic as it really would make the array a non-POD so initialisation would occur during the dynamic phase rather than the static phase. IMO that would be a deal braker :-(
John.
So an array class with a non-template assignment op is an aggregate but is not a POD. This language sometimes drives me nuts ;-) --Johan
Johan Råde wrote:
John Maddock wrote:
That one is problematic as it really would make the array a non-POD so initialisation would occur during the dynamic phase rather than the static phase.
The compiler is allowed to statically initialize anything it can, and in this case VC 7.1 does.
IMO that would be a deal braker :-(
We aren't actually testing static array initialization, are we? How do we know that it works at all? :-)
So an array class with a non-template assignment op is an aggregate but is not a POD. This language sometimes drives me nuts ;-)
Non-POD aggregates are common, consider struct X { std::string a, b; }; X x = { "a", "b" };
Peter Dimov wrote:
Johan Råde wrote:
John Maddock wrote:
That one is problematic as it really would make the array a non-POD so initialisation would occur during the dynamic phase rather than the static phase.
The compiler is allowed to statically initialize anything it can, and in this case VC 7.1 does.
True. Although there are other benefits that POD'ness brings: union support, pass through elipsis etc. However, since those are less useful, I might be persuaded that the fix is OK after all if VC7.1 really does the right thing by static initialisation.
IMO that would be a deal braker :-(
We aren't actually testing static array initialization, are we? How do we know that it works at all? :-)
Um, good question. It is very important though.
So an array class with a non-template assignment op is an aggregate but is not a POD. This language sometimes drives me nuts ;-)
Non-POD aggregates are common, consider
struct X { std::string a, b; };
X x = { "a", "b" };
Absolutely, and I've used boost::array with non-POD types as well as POD's. However, the ability to have POD's statically initialised "just like a C array" is a very useful one. John.
John Maddock wrote:
Peter Dimov wrote:
Johan Råde wrote:
John Maddock wrote:
That one is problematic as it really would make the array a non-POD so initialisation would occur during the dynamic phase rather than the static phase.
The compiler is allowed to statically initialize anything it can, and in this case VC 7.1 does.
True. Although there are other benefits that POD'ness brings: union support, pass through elipsis etc. However, since those are less useful, I might be persuaded that the fix is OK after all if VC7.1 really does the right thing by static initialisation.
IMO that would be a deal braker :-(
We aren't actually testing static array initialization, are we? How do we know that it works at all? :-)
Um, good question. It is very important though.
Adding a test case will accomplish three things: 1. It will show that we consider the feature important, 2. It will ensure that it works, 3. It will allow us to verify that patches don't break it. The nice thing is that all this will happen automatically without anybody needing to persuade anyone else. :-) Likewise, adding a test case for the original 7.1 problem could allow us to address its failure by other, more focused, means instead of going with the suggested fix. Test-first and all that. :-)
Peter Dimov wrote:
Adding a test case will accomplish three things:
1. It will show that we consider the feature important, 2. It will ensure that it works, 3. It will allow us to verify that patches don't break it.
The nice thing is that all this will happen automatically without anybody needing to persuade anyone else. :-)
Likewise, adding a test case for the original 7.1 problem could allow us to address its failure by other, more focused, means instead of going with the suggested fix.
Test-first and all that. :-)
However, I freely admit I have no idea how to test if something uses static initialization or dynamic. Actually, thinking on the fly ... Is everything due for dynamic initialization zero-initialized first? If so, I could declare sentry objects either side of the statically initialized array, and see if it contains zeroes in the first-but-not-second case. Actually, I could probably just check it has the expected values in the first case, zeroes or not. If it contains uninitialized random values it clearly also has not been statically initialized. Of course, this test only fails usefully on compilers that get order of initialization correct, but fail to notice that array< POD, N > should be statically initialized! -- AlisdairM
AlisdairM wrote:
However, I freely admit I have no idea how to test if something uses static initialization or dynamic.
Todd Greer already posted one possible test, but a much simpler version is
extern array
John Maddock wrote:
True. Although there are other benefits that POD'ness brings: union support, pass through elipsis etc.
Note that union support and PODness are different issues with a common-but-distinct set of requirements. For example ... struct base { int x; }; // aggregate/POD struct derived : base {}; derived is neither an aggregate nor a POD, but CAN be stored in a union. The requirement for a union are nothing to do with aggregates, but purely concern trivial constructor/destructor/copy-assign operator. struct agg { derived y; }; agg is an aggregate, is NOT a POD, but can still be stored in a union!. Similarly for an array of derived. Likewise, int is a classic POD that is not an aggregate than can be stored in a union. Most combinations of PODness, aggregate and union-ability can be achieved with effort. I have not cross-checked with ellipsis requirements yet to look for even more perverse combinations, just in case you did not think the language was confusing enough yet ;¬) [A very patient group of people at BSI have been waiting for me to write a paper on this for some time. Main problem is - all I can say right now is "ain't it awful!", which is not a very productive paper!] -- AlisdairM
Johan Råde wrote:
That one is problematic as it really would make the array a non-POD so initialisation would occur during the dynamic phase rather than the static phase. IMO that would be a deal braker :-(
John.
I agree with John. My proposed patch is dead. I think the Rationale section in the array documentation should be updated, to include what we learnt during this discussion. Something like: "The array class does not have a non-template assignment operator. If one were addded, the array class would still be an aggregate, and could still be initialized with a double brace enclosed initializer list. However, the array class would not be a POD anynmore, and such initialization would not be done statically." --Johan Råde
Johan Råde wrote:
I think the Rationale section in the array documentation should be updated, to include what we learnt during this discussion. Something like:
"The array class does not have a non-template assignment operator. If one were addded, the array class would still be an aggregate, and could still be initialized with a double brace enclosed initializer list. However, the array class would not be a POD anynmore, and such initialization would not be done statically."
--Johan Råde
The following might be better:
"The array class is designed to be an aggregate and a POD type.
Being an aggregate means that an array can be initialized with an
initializer list as follows:
boost::array
Johan Råde wrote:
The following might be better:
"The array class is designed to be an aggregate and a POD type. Being an aggregate means that an array can be initialized with an initializer list as follows:
boost::array
a = {{ 1, 2, 3 }}; Being a POD type ensure that such an initialization is done statically."
Currently the design rationale discusses the aggregate property but not the POD property. But both are important.
This has been a long and confused thread. I think much of the confusion has been caused by our failure, until today (thank you John), to realize that were two issies involved: aggregates and POD.
Thanks. The array docs need an update as soon as I get a little time spare to cover TR1 support as well. We should also note that array will be a POD so long as T is a POD ;¬) -- AlisdairM
participants (6)
-
AlisdairM
-
Daryle Walker
-
Johan Råde
-
John Maddock
-
Peter Dimov
-
Stephan Tolksdorf