Re: Unhealthy commit e3340714c8 Style: clarify "do nothing" macro with {};

[David Kastrup]

Graham Percival <address@hidden> writes:

> On Thu, Oct 7, 2010 at 8:36 PM, David Kastrup <address@hidden> wrote:
>>
>> -#define ASSERT_LIVE_IS_ALLOWED()
>> +#define ASSERT_LIVE_IS_ALLOWED() {};
>>
>> The problem is that the replaced statement defines
>> ASSERT_LIVE_IS_ALLOWED() as being 1, while the second defines it as {};.
>
>> That is a particular bad idea for things like
>>
>> if (condition) ASSERT_LIVE_IS_ALLOWED(); else ...
>
> My understanding was that this is a debugging construct which is only
> used as a function call.

That is why it should behave like one syntactically.

> I mean, the "non-else" definition of ASSERT_LIVE would also run into
> problems in that construct:
>
> #ifndef NDEBUG
> #define ASSERT_LIVE_IS_ALLOWED()     \
>   static bool passed_here_once;\
>   if (parsed_objects_should_be_dead && !passed_here_once) { \
>     ::programming_error (string ("Parsed object should be dead: ")  +
> __PRETTY_FUNCTION__ ); \
>     passed_here_once = true;\
>   }
> #else
> #define ASSERT_LIVE_IS_ALLOWED() {};
> #endif
>
> in that the macro might look like a single statement, but it gets
> expanded into two.  (is it worth enclosing the non-else definition
> with a { } to avoid this?)

"{ ... }" is not sufficient since that already is a complete statement
without adding a semicolon, unlike a function call.

The wrapper needs to be "do { ... } while (0)".  It is an idiom that
compilers optimize away.

>> So either ASSERT_LIVE_IS_ALLOWED() should be explicitly defined as 1.
>
> I'm fine with that change, if it makes sense given the non-else
> definition.  I'm kind-of out of my depth here.

I did not look at that.  Of course this concerns the other definition
masking as a function call equally well.

-- 
David Kastrup