On Sat, 24 Jun 2006 14:17:37 +0200, Gennaro Prota <gennaro_prota@yahoo.com> wrote:
Tricky question. IMHO it is undefined behavior. When BOOST_ASSERT falls back to the standard assert it really depends on the underlying library. C90 does not require that assert works with expressions having non-int type. In C99 it shall work for any scalar expression (which anyway "out of range" is not; it has array type).
We could perhaps protect against this kinds of things by using a static_cast in assert.hpp (probably documenting that)
I'd like to fix this problem. But I guess I should better explain that "perhaps". The static_cast would prevent using, for instance BOOST_STATIC_ASSERT("foo"); That's fine as long as we want the user to "perceive" the problem and be forced to write, for instance: BOOST_STATIC_ASSERT("foo" != 0 /*or nullptr*/); Otherwise the following choices come to mind: a) # define BOOST_ASSERT(expr) assert(0 != (expr)) b) # define BOOST_ASSERT(expr) assert(!!(expr)) c) # define BOOST_ASSERT(expr) assert("BOOST_LIBRARIES:" && (expr)) d) # define BOOST_ASSERT(expr) assert(!"BOOST_LIBRARIES:" || (expr)) The last one is for those who find "||" to be a better "separator" than "&&" :-o Looking forward for consensus and patching :) -- [ Gennaro Prota, C++ developer for hire ] [ resume: available on request ]
On Tue, 04 Jul 2006 15:12:43 +0200, Gennaro Prota <gennaro_prota@yahoo.com> wrote:
[snip]
Enclosed is some code revived (well, so to speak :-) from my personal libraries that could be added to our config tests; it does a few checks related to the standard assert headers and the corresponding macro definition. This doesn't mean we should have a BOOST_NO_CONFORMING_ASSERT macro, just that we could add it to provide information for the user. Other checks could be added, by the way. -- [ Gennaro Prota, C++ developer for hire ] [ resume: available on request ] begin 644 standard_assert_test.c M+RH@*$,I($=E;FYA<F\@4')O=&$@,3DY.0T*#0HO+R!$;V5S('9A<FEO=7,@ M:VEN9',@;V8@8VAE8VMS(&]N('1H92`\87-S97)T+F@^(&%N9"`\8V%S<V5R M=#X@:&5A9&5R<RX-"B\O($YO;F4@;V8@=&AE(&AE861E<G,@<VAA;&P@8F4@ M;6%C<F\@9W5A<F1E9"!A;F0@=VAE;B!.1$5"54<@:7,@9&5F:6YE9`T*+R\@ M87-S97)T*'@I('-H86QL(&5X<&%N9"!T;R`H*'9O:60I,"D-"B\O#0HO+R!. M3U1%.B!T:&ES(&ES(&%L<V\@82!S=')I8W1L>2!C;VYF;W)M:6YG($,Y,"!A M;F0@0SDY('!R;V=R86T-"B\O#0HO+R`M+2TM+2TM+2TM+2TM+2T-"B\O($EF M(&YO;F4@;V8@=&AE(&UA8W)O<R!W:&EC:"!E;F%B;&4@82!S<&5C:69I8R!T M97-T(&ES(&1E9FEN960L#0HO+R!T:&5N('1H92!C;V1E(&%T=&5M<'1S(&%L M;"!C:&5C:W,@:6X@<V5Q=65N8V4@86YD('=I;&P@86)O<G0H*0T*+R\@;W(@ M9F%I;"!T;R!C;VUP:6QE(&%T('1H92!F:7)S="!F86EL=7)E+"!S:VEP<&EN M9R!A;GD@<W5B<V5Q=65N=`T*+R\@8VAE8VMS+@T**B\-"@T*+RH@+2TM+2TM M+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2TM M+2TM+2TM+2TM("HO#0HC:68@(2!D969I;F5D($)/3U-47T-(14-+7TY$14)5 M1U]!4U-%4E0@(%P-"B`F)B`A9&5F:6YE9"!"3T]35%]#2$5#2U]!4U-%4E1? M2"`@("`@("`@7`T*("8F("%D969I;F5D($)/3U-47T-(14-+7T-!4U-%4E0@ M("`@("`@("!<#0H@)B8@(2!D969I;F5D($)/3U-47T-(14-+7T%34T525%]! M3$P@("`@(%P-"B`O*BHO#0HC(&1E9FEN92!"3T]35%]#2$5#2U]!4U-%4E1? M04Q,#0HC96YD:68-"@T*(VEF(&1E9FEN960@0D]/4U1?0TA%0TM?05-315)4 M7T%,3`T*(R!D969I;F4@0D]/4U1?0TA%0TM?3D1%0E5'7T%34T525`T*(R!D M969I;F4@0D]/4U1?0TA%0TM?05-315)47T@-"B,@9&5F:6YE($)/3U-47T-( M14-+7T-!4U-%4E0-"B-E;F1I9@T*#0H-"B\J("TM+2TM+2TM+2TM+2TM+2TM M+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2`J M+PT*+RH@("`@("`@("`@("`@("!.3U1%.B!S:&%L;"!B92!T:&4@9FER<W0@ M=&5S="`@("`@("`@("`@("`@("`@("HO#0HC:68@9&5F:6YE9"!"3T]35%]# M2$5#2U].1$5"54=?05-315)4#0HC("!U;F1E9B!.1$5"54<-"B,@(&1E9FEN M92!.1$5"54<-"B,@(&EN8VQU9&4@/&%S<V5R="YH/@T*#0H@("`@=F]I9"!N M;VY?9&5B=6=?;6%C<F]?9&5F:6YI=&EO;B@I#0H@("`@>R!A<W-E<G0H3D]. M7T-/3D9/4DU)3D=?1$5&24Y)5$E/3E]/1E]42$5?05-315)47TU!0U)/*3L@ M?0T*(V5L<V4-"@T*("`@('9O:60@;F]N7V1E8G5G7VUA8W)O7V1E9FEN:71I M;VXH*2![?0T*(V5N9&EF#0H-"@T*+RH@+2TM+2TM+2TM+2TM+2TM+2TM+2TM M+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2TM("HO#0HC M:68@9&5F:6YE9"!"3T]35%]#2$5#2U]!4U-%4E1?2`T*(R`@=6YD968@3D1% M0E5'#0HC("!I;F-L=61E(#QA<W-E<G0N:#X-"B,@(&1E9FEN92!.1$5"54<- M"B,@(&EN8VQU9&4@/&%S<V5R="YH/@T*#0H@("`@=F]I9"!A<W-E<G1?9W5A M<F1?8VAE8VLH*0T*("`@('L@87-S97)T*"));7!L96UE;G1A=&EO;B!H87,@ M;6%C<F\@9W5A<F0@;VX@/&%S<V5R="YH/B(@/3T@,"D[('T-"@T*(V5L<V4- M"@T*("`@('9O:60@87-S97)T7V=U87)D7V-H96-K*"D@>WT-"@T*(V5N9&EF M#0H-"B\J("TM+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2TM M+2TM+2TM+2TM+2TM+2TM+2TM+2TM+2`J+PT*(VEF(&1E9FEN960@0D]/4U1? M0TA%0TM?0T%34T525`T*(R`@=6YD968@3D1%0E5'#0HC("!I;F-L=61E(#QC M87-S97)T/@T*(R`@9&5F:6YE($Y$14)51PT*(R`@:6YC;'5D92`\8V%S<V5R M=#X-"@T*("`@('9O:60@8V%S<V5R=%]G=6%R9%]C:&5C:R@I#0H@("`@>R!A M<W-E<G0H(DEM<&QE;65N=&%T:6]N(&AA<R!M86-R;R!G=6%R9"!O;B`\8V%S M<V5R=#XB(#T](#`I.R!]#0HC96QS90T*#0H@("`@=F]I9"!C87-S97)T7V=U M87)D7V-H96-K*"D@>WT-"B-E;F1I9@T*#0H-"FEN="!M86EN("@I#0I[#0H@ M("`@87-S97)T7V=U87)D7V-H96-K*"D[#0H@("`@8V%S<V5R=%]G=6%R9%]C M:&5C:R@I.PT*("`@("\O;F]N7V1E8G5G7VUA8W)O7V1E9FEN:71I;VXH*3L@ L+R\@;F\@;F5E9"!T;R`B8V%L;"(-"@T*("`@(')E='5R;B`P.PT*?0T*#0H@ ` end
On Tue, 04 Jul 2006 15:12:43 +0200, Gennaro Prota <gennaro_prota@yahoo.com> wrote:
[something...]
I always wonder how a lack of answers should be taken: (a) nobody disagrees (b) nobody agrees (c) nobody cares. Hint: (c) is impossible, as *I* care, and for that reason I'll take (a) by default ;) -- [ Gennaro Prota, C++ developer for hire ] [ resume: available on request ]
Gennaro Prota wrote:
On Tue, 04 Jul 2006 15:12:43 +0200, Gennaro Prota <gennaro_prota@yahoo.com> wrote:
[something...]
I always wonder how a lack of answers should be taken: (a) nobody disagrees (b) nobody agrees (c) nobody cares.
Hint: (c) is impossible, as *I* care, and for that reason I'll take (a) by default ;)
BOOST_ASSERT wasn't supposed to be an improvement over assert, interface-wise. In my opinion, invalid assertions should be fixed; we should not alter BOOST_ASSERT to make them work.
On Wed, 5 Jul 2006 18:20:57 +0300, "Peter Dimov" <pdimov@mmltd.net> wrote:
BOOST_ASSERT wasn't supposed to be an improvement over assert, interface-wise. In my opinion, invalid assertions should be fixed; we should not alter BOOST_ASSERT to make them work.
I agree. The point, anyhow, was avoiding those invalid assertions. What I proposed above make them silently work as the user expects without invoking undefined behavior. The alternative is *catching* the error. If we can do the latter then it's even better to me. -- [ Gennaro Prota, C++ developer for hire ] [ resume: available on request ]
On Wed, 05 Jul 2006 18:05:20 +0200, Gennaro Prota <gennaro_prota@yahoo.com> wrote:
On Wed, 5 Jul 2006 18:20:57 +0300, "Peter Dimov" <pdimov@mmltd.net> wrote:
BOOST_ASSERT wasn't supposed to be an improvement over assert, interface-wise. In my opinion, invalid assertions should be fixed; we should not alter BOOST_ASSERT to make them work.
I agree. The point, anyhow, was avoiding those invalid assertions. What I proposed above make them silently work as the user expects without invoking undefined behavior. The alternative is *catching* the error. If we can do the latter then it's even better to me.
Just to spell out what I have in mind and make a couple of further questions (:-s): namespace boost { namespace detail { namespace assert_ { char valid_assert_expression(int); void valid_assert_expression(...); }}} #define BOOST_ASSERT(expr) \ ( \ (void)sizeof boost::detail::assert_ \ ::valid_assert_expression(expr) \ , assert(expr) \ ) /**/ Now I wonder: a) should we leave the test when BOOST_DISABLE_ASSERTS is defined too? b) why the last parameter of assertion_failed() is not unsigned long? -- [ Gennaro Prota, C++ developer for hire ] [ resume: available on request ]
On Thu, 6 Jul 2006 13:29:58 +0300, "Peter Dimov" <pdimov@mmltd.net> wrote:
Gennaro Prota wrote:
#define BOOST_ASSERT(expr) \ ( \ (void)sizeof boost::detail::assert_ \ ::valid_assert_expression(expr) \ , assert(expr) \ ) /**/
Sorry, I thought you had read my earlier post about this. I found some invocations of BOOST_ASSERT which don't use an expression with int type. When BOOST_ASSERT falls back to the standard assert macro it may work or not, depending on what the underlying C library does. See the last part of this message: <http://lists.boost.org/Archives/boost/2006/06/106721.php> Frequently the C library adds a 0 != in front of (expr), uses a ?: operator, or ||, so that everything works as expected (and, in fact, C99 extends the specification to allow any scalar expression --as that's easy to deal with for the library). But that's not guaranteed. -- [ Gennaro Prota, C++ developer for hire ] [ resume: available on request ]
Gennaro Prota wrote:
On Thu, 6 Jul 2006 13:29:58 +0300, "Peter Dimov" <pdimov@mmltd.net> wrote:
Gennaro Prota wrote:
#define BOOST_ASSERT(expr) \ ( \ (void)sizeof boost::detail::assert_ \ ::valid_assert_expression(expr) \ , assert(expr) \ ) /**/
Sorry, I thought you had read my earlier post about this. I found some invocations of BOOST_ASSERT which don't use an expression with int type. When BOOST_ASSERT falls back to the standard assert macro it may work or not, depending on what the underlying C library does.
I know. Is this a real problem? Are you sure that making BOOST_ASSERT fail when assert works a good idea? It will just encourage people to not use it. Not that they need much encouragement. Almost everyone ignores BOOST_ASSERT (and the users' requests to please not do so).
On Thu, 6 Jul 2006 14:27:57 +0300, "Peter Dimov" <pdimov@mmltd.net> wrote:
I know. Is this a real problem?
Well, doesn't seem false to me :)
Are you sure that making BOOST_ASSERT fail when assert works a good idea?
It happens to work, and relies on undefined behavior. I think it's a good idea to catch that.
It will just encourage people to not use it. Not that they need much encouragement. Almost everyone ignores BOOST_ASSERT (and the users' requests to please not do so).
I'll assume you mean "not to ignore". Well, all I can promise is that I'll change all asserts in dynamic_bitset to BOOST_ASSERT. I can't do more, sorry. While we are at it, I've never liked too much the idea to switch off (or on) all asserts from any boost library. I would often desire a higher degree of control; one thing I can imagine is passing type information to assertion_failed(), so that I can see what library the assert is coming from and decide what to do with that. -- [ Gennaro Prota, C++ developer for hire ] [ resume: available on request ]
Gennaro Prota wrote:
On Thu, 6 Jul 2006 14:27:57 +0300, "Peter Dimov" <pdimov@mmltd.net> wrote:
I know. Is this a real problem?
Well, doesn't seem false to me :)
I didn't imply that it was false. I asked whether this is a real problem.
Are you sure that making BOOST_ASSERT fail when assert works a good idea?
It happens to work, and relies on undefined behavior. I think it's a good idea to catch that.
BOOST_ASSERT is documented to resolve to assert by default. There is nothing undefined about it.
It will just encourage people to not use it. Not that they need much encouragement. Almost everyone ignores BOOST_ASSERT (and the users' requests to please not do so).
I'll assume you mean "not to ignore". Well, all I can promise is that I'll change all asserts in dynamic_bitset to BOOST_ASSERT. I can't do more, sorry.
While we are at it, I've never liked too much the idea to switch off (or on) all asserts from any boost library.
This is a bit better than not being able to switch off all Boost asserts without also switching off all non-Boost asserts, isn't it? :-) You can of course define BOOST_DYNAMIC_BITSET_ASSERT that offers a finer degree of control, and I'm sure that some of your users will appreciate it. BOOST_ASSERT is just a baseline replacement for assert that attempts to strike a balance between the needs of the library developers (who would rather just use assert without thinking) and the needs of the library users (who at minimum need to be able to disable or trap all library assertions.) One important design goal of BOOST_ASSERT is for someone to be able to do a global search and replace in the Boost codebase, turning all 'assert's to BOOST_ASSERTs. Unfortunately, BOOST_ASSERT doesn't work even in this minimalistic form; most developers just don't care. But at least the users who do care can do the global replace themselves. Do not make them jump through additional hoops just because.
I would often desire a higher degree of control; one thing I can imagine is passing type information to assertion_failed(), so that I can see what library the assert is coming from and decide what to do with that.
I'm not sure where this type information can come from, as BOOST_ASSERT is only given an expression.
On Thu, 6 Jul 2006 16:21:57 +0300, "Peter Dimov" <pdimov@mmltd.net> wrote:
I didn't imply that it was false. I asked whether this is a real problem.
I know :) I just wanted to say that it *is* a problem, and we can do something *easy* about it. I agree that it is not BOOST_ASSERT's responsibility to fix user/developer's errors, but a check wouldn't hurt anyone.
Are you sure that making BOOST_ASSERT fail when assert works a good idea?
It happens to work, and relies on undefined behavior. I think it's a good idea to catch that.
BOOST_ASSERT is documented to resolve to assert by default. There is nothing undefined about it.
Yeah, there's nothing undefined about BOOST_ASSERT. It's about *assert*: invoking it with a parameter which doesn't end up in an integral expression produces undefined behavior (when NDEBUG is not defined).
It will just encourage people to not use it.
Well, BOOST_ASSERT is aimed at developers. I can't believe a programmer wouldn't use it because it pointed out an error in his code that he doesn't want to accept. And all that he needs doing to go on is to add a "!=0" in 99.9% of the cases.
[...]
While we are at it, I've never liked too much the idea to switch off (or on) all asserts from any boost library.
This is a bit better than not being able to switch off all Boost asserts without also switching off all non-Boost asserts, isn't it? :-)
Sure.
You can of course define BOOST_DYNAMIC_BITSET_ASSERT that offers a finer degree of control, and I'm sure that some of your users will appreciate it.
Then if every library author chooses his own solution... I was looking for consistency, which is the last refuge of the unimaginative. -- [ Gennaro Prota, C++ developer for hire ] [ resume: available on request ]
Gennaro Prota wrote:
On Thu, 6 Jul 2006 16:21:57 +0300, "Peter Dimov" <pdimov@mmltd.net> wrote:
I didn't imply that it was false. I asked whether this is a real problem.
I know :) I just wanted to say that it *is* a problem, and we can do something *easy* about it. I agree that it is not BOOST_ASSERT's responsibility to fix user/developer's errors, but a check wouldn't hurt anyone.
A check will hurt everyone who does a global assert -> BOOST_ASSERT replace and the code refuses to compile without a good reason.
BOOST_ASSERT is documented to resolve to assert by default. There is nothing undefined about it.
Yeah, there's nothing undefined about BOOST_ASSERT. It's about *assert*: invoking it with a parameter which doesn't end up in an integral expression produces undefined behavior (when NDEBUG is not defined).
You actually made me check. :-) This is not true for C99: it requires a scalar type, not an integral expression. So, unless we are 100% sure that the underlying assert macro isn't C99-compatible (and my guess is that most of them are), we can't diagnose this as an error. I don't have C90, so I take your word for it.
You can of course define BOOST_DYNAMIC_BITSET_ASSERT that offers a finer degree of control, and I'm sure that some of your users will appreciate it.
Then if every library author chooses his own solution... I was looking for consistency, which is the last refuge of the unimaginative.
Providing a consistent mechanism for per-library assertion control is probably technically doable, but this wasn't a design goal of BOOST_ASSERT. If you have a proposal along those lines (we can call it BOOST_LIBRARY_ASSERT, for instance), it might be worth considering.
On Thu, 6 Jul 2006 18:11:40 +0300, "Peter Dimov" <pdimov@mmltd.net> wrote:
A check will hurt everyone who does a global assert -> BOOST_ASSERT replace and the code refuses to compile without a good reason.
:-/
[...]
Yeah, there's nothing undefined about BOOST_ASSERT. It's about *assert*: invoking it with a parameter which doesn't end up in an integral expression produces undefined behavior (when NDEBUG is not defined).
You actually made me check. :-) This is not true for C99
I thought I new :-( <http://lists.boost.org/Archives/boost/2006/06/106721.php> <http://lists.boost.org/Archives/boost/2006/07/107371.php>
[...]
I don't have C90, so I take your word for it.
Thanks. -- [ Gennaro Prota, C++ developer for hire ] [ resume: available on request ]
Gennaro Prota wrote:
I thought I new :-(
<http://lists.boost.org/Archives/boost/2006/06/106721.php> <http://lists.boost.org/Archives/boost/2006/07/107371.php>
Indeed, sorry for not paying attention.
"Peter Dimov" <pdimov@mmltd.net> writes:
Gennaro Prota wrote:
On Thu, 6 Jul 2006 13:29:58 +0300, "Peter Dimov" <pdimov@mmltd.net> wrote:
Gennaro Prota wrote:
#define BOOST_ASSERT(expr) \ ( \ (void)sizeof boost::detail::assert_ \ ::valid_assert_expression(expr) \ , assert(expr) \ ) /**/
Sorry, I thought you had read my earlier post about this. I found some invocations of BOOST_ASSERT which don't use an expression with int type. When BOOST_ASSERT falls back to the standard assert macro it may work or not, depending on what the underlying C library does.
I know. Is this a real problem? Are you sure that making BOOST_ASSERT fail when assert works a good idea? It will just encourage people to not use it. Not that they need much encouragement. Almost everyone ignores BOOST_ASSERT (and the users' requests to please not do so).
Alright, I'd rather not read such bitter regrets, so what can we do about that? Maybe it's time to more seriously revive the idea of Boost programming guidelines. I'm amazed that we've come this far without a more complete document than what's provided at http://www.boost.org/more/lib_guide.htm#Design_and_Programming -- Dave Abrahams Boost Consulting www.boost-consulting.com
On Thu, 06 Jul 2006 09:57:47 -0400, David Abrahams <dave@boost-consulting.com> wrote:
Maybe it's time to more seriously revive the idea of Boost programming guidelines. I'm amazed that we've come this far without a more complete document than what's provided at http://www.boost.org/more/lib_guide.htm#Design_and_Programming
It was (and is, IMO) a very good guide. It's just a bit unmaintained. I think mentioning BOOST_ASSERT there, and maybe in http://www.boost.org/more/error_handling.html too, might help. -- [ Gennaro Prota, C++ developer for hire ] [ resume: available on request ]
On Thu, 06 Jul 2006 09:57:47 -0400, David Abrahams <dave@boost-consulting.com> wrote:
Maybe it's time to more seriously revive the idea of Boost programming guidelines.
In the last days I've had a chance to look in places of the boost source tree were I had never been before, to fix Boost Inspect issues. Sorry to say, it's time to seriously introduce code review. OTOH, I realize we are already low on resource enough to add anything else to the mix :-( -- [ Gennaro Prota, C++ developer for hire ]
participants (3)
-
David Abrahams -
Gennaro Prota -
Peter Dimov