On Sat, Jan 31, 2009 at 4:05 PM, Giel van Schijndel <m...@mortis.eu> wrote: > I like the general idea. About your patch, assert() itself performs > string expansion on the given expression as well (for display purposes). > Doesn't caching the expression's result cause assert() to see another > string, i.e. just "_wzeval"?
No. There are in fact three places now that the expression result is checked: 1) Checking if we should log an error 2) The assert() 3) The return condition In my patch I only cache it in the first case for the last case, since the second case will be void in non-debug builds anyway. But we should consider just doing assert(false) since we log the expression and error better in the code right above it anyway. > Additionally, I think we shouldn't have any code at all that depends on > expressions inside of ASSERT()/assert()/etc. to have side effects. We are totally in agreement on that point. However, that is not why I do want to cache it. In some cases, quite a few actually, the expression calls a validation function that may take non-trivial amounts of time to process. I do not want us to have to worry about such issues when we write assert tests. Both with and without the patch, we check the expression twice in debug builds, and once in non-debug builds (for logging). We may want to consider making ASSERT() a noop in non-debug builds, and ASSERT_RETURN only check it once for return, to make sure we get maximum speed. > Additionally I think ASSERT_OR_RETURN is a slightly better name It is misleading, though, since it can also be ASSERT_AND_RETURN or just RETURN, depending on how the program is compiled and how gdb reacts to abort signals. So I would prefer to keep the slight ambiguity in the name. - Per _______________________________________________ Warzone-dev mailing list Warzone-dev@gna.org https://mail.gna.org/listinfo/warzone-dev
