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]

[Dean Long]

On 2/19/2013 11:58 PM, Péter Radics wrote:
>
> On Wed, Feb 20, 2013 at 7:33 AM, Dean Long <dean.long at oracle.com 
> <mailto: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
>

A less disruptive change would be to add something like:

#undef assert
#define assert(e,msg) assert_vm(e,msg)

after the #includes in all hotspot source files.  Hopefully this would 
have little impact on back-ports and patches.

dl