8007770: fix name clash of assert macro in debug.hpp with libc's assert macro [Was: Re: Bug 100297 - [patch] fix name clash of assert macro in debug.hpp with libc's assert macro]

[Péter Radics]

On Wed, Feb 20, 2013 at 7:33 AM, Dean Long <dean.long at oracle.com> wrote:

> On 2/19/2013 6:44 PM, David Holmes wrote:
>
>> On 20/02/2013 7:56 AM, Christian Thalinger wrote:
>>
>>> [David already filed an RFE for this:
>>>
>>> 8007770: fix name clash of assert macro in debug.hpp with libc's assert
>>> macro]
>>>
>>
>> Yes. For the record it is only undefined behaviour if you suppress the
>> assert macro and re-define it as a function - which we do not do.
>>
>> John Rose also added the following to the bug report:
>>
>> "I count about 15,000 occurrences of assert in our sources.
>> The proposed rename would therefore be a disruptive change.
>> Patches and change sets would no longer apply across that change, leading
>> to maintenance costs, especially back-porting bug fixes.
>> Thus, a mere claim of "would be prudent to conform exactly to the
>> standard" is not enough grounds to accept this change.
>>
>> Since 'assert' macros are common programming practice, we can expect (or
>> request, if necessary) that the C++ standard libraries provide an op-out
>> mechanism, such as '#undef assert' or '-DNDEBUG'.
>>
>> For a reference on NDEBUG, see:
>> http://www.cplusplus.com/reference/cassert/assert/
>>
>> (To be clear, NDEBUG does not remove the macro. Another option would be
>> required, if #undef is not allowed.) "
>>
>> ---
>>
>> If this is potentially a problem then I think we need to avoid it by
>> other means - most likely by bracketing standard includes with "#undef
>> assert" so that they are not affected by hotspot's definition, and our
>> source is not affected by the definition from assert.h
>>
>>
> Can't we just do something like the following in debug.hpp?
>
> #include <assert.h>
> #undef assert
> #define assert(p, msg) \
> ...
>
> Any #include <assert.h> that happens after this should be a no-op because
> it was already included once.
>
> dl
>
>  David
>> -----
>>
>
Unfortunately that doesn't work, as <assert.h> may be included multiple
times by design, even with different NDEBUG settings.

Regarding the 15k occurrences of assert: yes, it's unfortunate, and it is a
disruptive change, but other workarounds (such as the one attempted with
shark/llvmHeaders.hpp) are ugly and brittle, so in my opinion this one-time
disruption should be accepted.

Incoming patches could be pre-processed with a script like the one I
attached to the bug report in the transition-period, but I think most repos
will pick up the change within a reasonable time.

The other options is to try convincing the libc++ devs to not use a
perfectly valid way of using the standard assert facilities...

cheers,
mitch