[systemd-devel] [PATCH 2/2] macro: allow assert_se() assertions to also be optimized out when NDEBUG is set

[Lennart Poettering]

On Thu, 26.03.15 23:18, Shawn Landden (shawn at churchofgit.com) wrote:

> On Thu, Mar 26, 2015 at 1:19 AM, Lennart Poettering
> <lennart at poettering.net> wrote:
> > On Tue, 24.03.15 11:11, Shawn Landden (shawn at churchofgit.com) wrote:
> >
> >> Will result in slightly smaller binaries, and cuts out the branch, even if
> >> the expression is still executed.
> >
> > I am sorry, but the whole point of assert_se() is that it has side
> > effects. That's why it is called that way (the "se" suffix stands for
> > "side effects"), and is different from assert(). You are supposed to
> > use it whenever the code enclosed in it shall not be suppressed if
> > NDEBUG is defined. This patch of yours breaks that whole logic!
> 
> +#define assert_se(expr) do {expr} while(false)
>  #define assert(expr) do {} while(false)
> 
> No it does not. see "{expr}", it still is doing the side effect. It
> just doesn't check if the return value is as expected.

I see.

Well, in general I think:

a) actually using NDEBUG is crazy. It's mostly theoretical thing, not
   something people should really do.

b) we don't optimize 3k or 7k away. It's negligible.

c) We don't optimize error paths. Optimization we only do for inner
   loops. 

> >>  #ifdef NDEBUG
> >> +#define assert_se(expr) do {expr} while(false)

Does this even work? I mean, lines within the {} of a do/while block
need to end in a semicolon to be useful. Did you actually test this
with -DNDEBUG during compilation?

Also, if somebody uses this:

      assert_se(a == 7);

Then you turn this into

      do { a == 7 } while(false);

Which (ignoring the fact that the semicolon is missing) will result in
a compiler warning, given that we make a comparison without using its
result. The only way this could work would be this:

      #define assert_se(expr) do { (void) (expr) } while(false)

i.e. explicitly casting the result of the expression to (void)...

That all said, I am not convinced that it is worth doing this change
at all.

-- 
Lennart Poettering, Red Hat