additional test for is_abstract type trait
As I mentioned in previous posts the following code program #include <boost/type_traits/is_abstract.hpp> #include <iostream> template<typename T> struct A { virtual void f() = 0; } int main() { if(boost::is_abstract<A<int> >::value) { std::cout << "A<int> is abstract"; else { std::cout << "A<int> is not abstract"; } } erroneously outputs "A<int> is not abstract" if compiled with gcc 3.4. However, the regression tests report that gcc 3.4 passes the is_abstract test. The reason is that above error is triggered only if A<int> is not instantiated by the compiler; but is_abstract_test.cpp does not test class template specializations at all. This compiler bug cost me quite some time when my application showed strange errors within the boost serialization library. Therefore, I'd like to ensure that boost developers are aware of this issue so similar problems can be avoided in the future. I thus propose to add the attached test file to the type_trait regression tests. The file is based on the existing is_abstract_test.cpp. My contribution is merely that I turned all structs into templates and that I added some extra tests w.r.t. explicit spezialization. I therefore left the copyright statement as I found it in is_abstract_test.cpp. If you add the file to the CVS, I leave it to you whether you add my name there; I don't care much either way. Regards Christoph -- http://www.informatik.tu-darmstadt.de/TI/Mitarbeiter/cludwig.html LiDIA: http://www.informatik.tu-darmstadt.de/TI/LiDIA/Welcome.html
Christoph Ludwig <cludwig@cdc.informatik.tu-darmstadt.de> writes:
As I mentioned in previous posts the following code program
#include <boost/type_traits/is_abstract.hpp> #include <iostream>
template<typename T> struct A { virtual void f() = 0; }
int main() { if(boost::is_abstract<A<int> >::value) { std::cout << "A<int> is abstract"; else { std::cout << "A<int> is not abstract"; } }
erroneously outputs "A<int> is not abstract" if compiled with gcc 3.4.
However, the regression tests report that gcc 3.4 passes the is_abstract test. The reason is that above error is triggered only if A<int> is not instantiated by the compiler; but is_abstract_test.cpp does not test class template specializations at all.
Why don't we just get is_abstract to cause the instantiation? It seems to me that you'd need to do that anyway in order to detect abstractness. As a matter of fact, I have some doubts about your diagnosis, since I can't imagine any way that is_abstract could work without doing the instantiation. -- Dave Abrahams Boost Consulting http://www.boost-consulting.com
On Thu, Nov 11, 2004 at 10:07:43AM -0500, David Abrahams wrote:
Christoph Ludwig <cludwig@cdc.informatik.tu-darmstadt.de> writes:
As I mentioned in previous posts the following code program
#include <boost/type_traits/is_abstract.hpp> #include <iostream>
template<typename T> struct A { virtual void f() = 0; }
int main() { if(boost::is_abstract<A<int> >::value) { std::cout << "A<int> is abstract"; else { std::cout << "A<int> is not abstract"; } }
erroneously outputs "A<int> is not abstract" if compiled with gcc 3.4.
However, the regression tests report that gcc 3.4 passes the is_abstract test. The reason is that above error is triggered only if A<int> is not instantiated by the compiler; but is_abstract_test.cpp does not test class template specializations at all.
Why don't we just get is_abstract to cause the instantiation? It seems to me that you'd need to do that anyway in order to detect abstractness. As a matter of fact, I have some doubts about your diagnosis, since I can't imagine any way that is_abstract could work without doing the instantiation.
Well, it's really Giovanni Bajo's diagnosis who implemented the support for DR 337 in gcc 3.4, IIUC. So I am going to trust him on this... :-) There was agreement on gcc bugzilla that section 14.7.1/p4 of the standard requires the instantiation of the class template and that gcc is in error not to do so. But this bug won't be fixed in the 3.4 release series. I will give a workaround within is_abstract that forces the instantiation some thought later in the evening. But even if I come up with a patch, I think it is worth explicitly testing whether is_abstract interacts well with templates. This compiler bug has shown again that errors creep in where you don't expect them. Regards Christoph -- http://www.informatik.tu-darmstadt.de/TI/Mitarbeiter/cludwig.html LiDIA: http://www.informatik.tu-darmstadt.de/TI/LiDIA/Welcome.html
On Thu, Nov 11, 2004 at 04:37:53PM +0100, Christoph Ludwig wrote:
On Thu, Nov 11, 2004 at 10:07:43AM -0500, David Abrahams wrote:
Christoph Ludwig <cludwig@cdc.informatik.tu-darmstadt.de> writes: [...]
However, the regression tests report that gcc 3.4 passes the is_abstract test. The reason is that above error is triggered only if A<int> is not instantiated by the compiler; but is_abstract_test.cpp does not test class template specializations at all.
Why don't we just get is_abstract to cause the instantiation? It seems to me that you'd need to do that anyway in order to detect abstractness. As a matter of fact, I have some doubts about your diagnosis, since I can't imagine any way that is_abstract could work without doing the instantiation.
attached is: 1) a corrected version of is_abstract_template_test.cpp When I prepared the original version I did not realize that is_abstract<T&>::value is supposed to be false for any type T. 2) a patch to boost/type_traits/is_abstract.hpp that works around gcc bug #17232. I would not be surprised if my patch turns out to be overly complicated. I had assumed that any expression that involves sizeof(T) causes the instantiation of T if T is a template specialization. So I tried, e.g., to add the condition sizeof(T) > 0 to the parameter list of ice_and in is_abstract_imp. For some reason, though, whatever I tried showed no effect. (Perhaps the compiler realized that the conditions I added were true for any type T and eliminated the evaluation of sizeof(T)?) I finally settled for attached patch that adds a further struct in which SFINAE is used again. With this patch gcc 3.4.2 passes both is_abstract_test.cpp as well as the attached is_abstract_template_test.cpp. I still would like to see something like is_abstract_template_test.cpp added to the type_traits tests. It is easy to introduce subtle bugs both in the compiler when implementing SFINAE and template instantiation as well as in the code relying on SFINAE. Regards Christoph -- http://www.informatik.tu-darmstadt.de/TI/Mitarbeiter/cludwig.html LiDIA: http://www.informatik.tu-darmstadt.de/TI/LiDIA/Welcome.html
1) a corrected version of is_abstract_template_test.cpp
When I prepared the original version I did not realize that is_abstract<T&>::value is supposed to be false for any type T.
2) a patch to boost/type_traits/is_abstract.hpp that works around gcc bug #17232.
I would not be surprised if my patch turns out to be overly complicated. I had assumed that any expression that involves sizeof(T) causes the instantiation of T if T is a template specialization. So I tried, e.g., to add the condition sizeof(T) > 0 to the parameter list of ice_and in is_abstract_imp. For some reason, though, whatever I tried showed no effect. (Perhaps the compiler realized that the conditions I added were true for any type T and eliminated the evaluation of sizeof(T)?)
I finally settled for attached patch that adds a further struct in which SFINAE is used again.
With this patch gcc 3.4.2 passes both is_abstract_test.cpp as well as the attached is_abstract_template_test.cpp.
I still would like to see something like is_abstract_template_test.cpp added to the type_traits tests. It is easy to introduce subtle bugs both in the compiler when implementing SFINAE and template instantiation as well as in the code relying on SFINAE.
I agree, sorry for not getting on top of this, but I will add your tests into the type traits test suite at some point (and the patch as well obviously). Thanks for this, John.
1) a corrected version of is_abstract_template_test.cpp
I've updated is_abstract test with your tests, found that VC7.1 has the same bug (?) as gcc-3.4 BTW.
2) a patch to boost/type_traits/is_abstract.hpp that works around gcc bug #17232.
I would not be surprised if my patch turns out to be overly complicated. I had assumed that any expression that involves sizeof(T) causes the instantiation of T if T is a template specialization. So I tried, e.g., to add the condition sizeof(T) > 0 to the parameter list of ice_and in is_abstract_imp. For some reason, though, whatever I tried showed no effect. (Perhaps the compiler realized that the conditions I added were true for any type T and eliminated the evaluation of sizeof(T)?)
I finally settled for attached patch that adds a further struct in which SFINAE is used again.
I've simplified your fix down to a: BOOST_STATIC_ASSERT(sizeof(T)); which also does the trick quite nicely. I'm running all the type traits tests through now, as long as there are no unexpected hiccups it'll all be in cvs soon. John.
On Mon, Nov 29, 2004 at 04:04:29PM -0000, John Maddock wrote:
1) a corrected version of is_abstract_template_test.cpp
I've updated is_abstract test with your tests, found that VC7.1 has the same bug (?) as gcc-3.4 BTW.
2) a patch to boost/type_traits/is_abstract.hpp that works around gcc bug #17232.
[...] I've simplified your fix down to a:
BOOST_STATIC_ASSERT(sizeof(T));
which also does the trick quite nicely.
I'm running all the type traits tests through now, as long as there are no unexpected hiccups it'll all be in cvs soon.
Thanks! Christoph -- http://www.informatik.tu-darmstadt.de/TI/Mitarbeiter/cludwig.html LiDIA: http://www.informatik.tu-darmstadt.de/TI/LiDIA/Welcome.html
John, now that you're at is_abstract, I wonder whether you could consider adding some workaround in is_abstract.hpp making boost::is_abstract return unconditionally false for those compilers known not to work for it. You can read about the origin of this request at: http://aspn.activestate.com/ASPN/Mail/Message/boost/2209389 In a nutshell, Boost.Serialization does some (ODR-nonconformant) redefinition of boost::is_abstract the way I'm suggesting. These precautions are necessary because, on those compilers not supporting the is_abstract implementation, the construct can crash instead of returning false, and this makes it impossible to use the facility without carefully taking care of the compiler. The hack at Boost.Serialization is ugly as sin, it is dependant on the order of header inclusion, violates ODR, etc., but if embedded directly into is_abstract it'd be just fine, AFAICS. Joaquín M López Muñoz Telefónica, Investigación y Desarrollo John Maddock ha escrito:
1) a corrected version of is_abstract_template_test.cpp
I've updated is_abstract test with your tests, found that VC7.1 has the same bug (?) as gcc-3.4 BTW.
2) a patch to boost/type_traits/is_abstract.hpp that works around gcc bug #17232.
I would not be surprised if my patch turns out to be overly complicated. I had assumed that any expression that involves sizeof(T) causes the instantiation of T if T is a template specialization. So I tried, e.g., to add the condition sizeof(T) > 0 to the parameter list of ice_and in is_abstract_imp. For some reason, though, whatever I tried showed no effect. (Perhaps the compiler realized that the conditions I added were true for any type T and eliminated the evaluation of sizeof(T)?)
I finally settled for attached patch that adds a further struct in which SFINAE is used again.
I've simplified your fix down to a:
BOOST_STATIC_ASSERT(sizeof(T));
which also does the trick quite nicely.
I'm running all the type traits tests through now, as long as there are no unexpected hiccups it'll all be in cvs soon.
John.
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
"Joaquín Mª López Muñoz" wrote:
John, now that you're at is_abstract, I wonder whether you could consider adding some workaround in is_abstract.hpp making boost::is_abstract return unconditionally false for those compilers known not to work for it. You can read about the origin of this request at:
Wouldn't it be better to have macro like BOOST_NO_XYZ defined for failing compilers instead? (I once suggested BOOST_NO_SUPPORT_FOR_DR337 but it was seen less then descriptive.) /Pavel
John, now that you're at is_abstract, I wonder whether you could consider adding some workaround in is_abstract.hpp making boost::is_abstract return unconditionally false for those compilers known not to work for it. You can read about the origin of this request at:
http://aspn.activestate.com/ASPN/Mail/Message/boost/2209389
In a nutshell, Boost.Serialization does some (ODR-nonconformant) redefinition of boost::is_abstract the way I'm suggesting. These precautions are necessary because, on those compilers not supporting the is_abstract implementation, the construct can crash instead of returning false, and this makes it impossible to use the facility without carefully taking care of the compiler.
Uh, I hadn't realised that that was the case. However, here's the question: is it always true that returning false is always safe? If the user's code uses is_abstract to determine whether an actual instance of a class can exist, then won't it do the wrong thing? I can (just about) understand is_polymorphic having a safe default, but is that true of is_abstract? I suppose what I'm saying is that the users idea of what constitutes a safe default may vary from use case to use case. We should define an appropriate macro though, whatever else is decided. John.
John Maddock ha escrito:
John, now that you're at is_abstract, I wonder whether you could consider adding some workaround in is_abstract.hpp making boost::is_abstract return unconditionally false for those compilers known not to work for it. You can read about the origin of this request at:
http://aspn.activestate.com/ASPN/Mail/Messae/boost/2209389
In a nutshell, Boost.Serialization does some (ODR-nonconformant) redefinition of boost::is_abstract the way I'm suggesting. These precautions are necessary because, on those compilers not supporting the is_abstract implementation, the construct can crash instead of returning false, and this makes it impossible to use the facility without carefully taking care of the compiler.
Uh, I hadn't realised that that was the case. However, here's the question: is it always true that returning false is always safe? If the user's code uses is_abstract to determine whether an actual instance of a class can exist, then won't it do the wrong thing? I can (just about) understand is_polymorphic having a safe default, but is that true of is_abstract?
I suppose what I'm saying is that the users idea of what constitutes a safe default may vary from use case to use case.
We should define an appropriate macro though, whatever else is decided.
Yes. Pavel proposes to add this informative macro and *no* workaround, so that it's up to the user to use is_abstract in defective compilers (even risking a crash) or build some internal logic to avoid it based on the macro. Thinking about it, I tend to agree with Pavel and you: returning false is not necessarily a reasonable default, so I'd go for the macro, only (and ask Robert to fix his hack in Boost.Serialization.) Joaquín M López Muñoz Telefónica, Investigación y Desarrollo
Joaquín Mª López Muñoz wrote:
Yes. Pavel proposes to add this informative macro and *no* workaround, so that it's up to the user to use is_abstract in defective compilers (even risking a crash) or build some internal logic to avoid it based on the macro.
Thinking about it, I tend to agree with Pavel and you: returning false is not necessarily a reasonable default, so I'd go for the macro, only (and ask Robert to fix his hack in Boost.Serialization.)
It seems to me that true is the appropriate safe default for is_abstract. Am I missing something? -- Dave Abrahams Boost Consulting http://www.boost-consulting.com
David Abrahams ha escrito:
Joaquín Mª López Muñoz wrote:
Yes. Pavel proposes to add this informative macro and *no* workaround, so that it's up to the user to use is_abstract in defective compilers (even risking a crash) or build some internal logic to avoid it based on the macro.
Thinking about it, I tend to agree with Pavel and you: returning false is not necessarily a reasonable default, so I'd go for the macro, only (and ask Robert to fix his hack in Boost.Serialization.)
It seems to me that true is the appropriate safe default for is_abstract. Am I missing something?
So that generic code won't try to instantiate an abstract type? Is this your rationale? Well, maybe. In the case of Boost.Serialization, however, I think that the appropriate default is false, since serializating (types derived from) an abstract type is more difficult than the non-abstract case. Either way, if we have a BOOST_NO_IS_ABSTRACT (for instance) macro, the programmer can choose whether to rely on is_abstract's default of not. Joaquín M López Muñoz Telefónica, Investigación y Desarrollo
Joaquín Mª López Muñoz wrote:
David Abrahams ha escrito:
It seems to me that true is the appropriate safe default for is_abstract. Am I missing something?
So that generic code won't try to instantiate an abstract type? Is this your rationale?
Yes.
Well, maybe. In the case of Boost.Serialization, however, I think that the appropriate default is false, since serializating (types derived from) an abstract type is more difficult than the non-abstract case.
But serialization can always check the macro. The standard practice for type traits that need some compiler support is to degrade gracefully (code still compiles) and safely (only the most conservative assumptions are made). Why shouldn't we do that in this case?
Either way, if we have a BOOST_NO_IS_ABSTRACT (for instance) macro, the programmer can choose whether to rely on is_abstract's default of not.
Yes, but that's a different issue. -- Dave Abrahams Boost Consulting http://www.boost-consulting.com
David Abrahams ha escrito:
Joaquín Mª López Muñoz wrote:
David Abrahams ha escrito:
It seems to me that true is the appropriate safe default for is_abstract. Am I missing something?
So that generic code won't try to instantiate an abstract type? Is this your rationale?
Yes.
Well, maybe. In the case of Boost.Serialization, however, I think that the appropriate default is false, since serializating (types derived from) an abstract type is more difficult than the non-abstract case.
But serialization can always check the macro. The standard practice for type traits that need some compiler support is to degrade gracefully (code still compiles) and safely (only the most conservative assumptions are made). Why shouldn't we do that in this case?
Yes, maybe. I don't have more arguments in favor of either option that I've already given, so defaulting to true will be just fine too (if documented). What I'd like to have is the macro, as I said, so that the hack at Boost.Serialization can be taken care of. (Not that I abhor of hacks, the problem with the current one is that it is dependent on the order of header inclusions. I'm surprised that no user has reported this yet.) (I've searched the web for real-world use cases of is_abstract that can help us decide about the best default value. Turns out I've found no code using it except Boost.Serialization.)
Either way, if we have a BOOST_NO_IS_ABSTRACT (for instance) macro, the programmer can choose whether to rely on is_abstract's default of not.
Yes, but that's a different issue.
?? By BOOST_NO_IS_ABSTRACT I mean a config macro telling whether DR337 is supported. It's the same issue, or else I'm not getting you. Joaquín M López Muñoz Telefónica, Investigación y Desarrollo
participants (5)
-
Christoph Ludwig -
David Abrahams -
Joaquín Mª López Muñoz -
John Maddock -
Pavel Vozenilek