Document assert_(), combine with __ASSERT_; fixup Perl_assert() #23543
Conversation
This comment is not true; it may have been true once. assert.h is always included, and assert() is always defined, since at least C89..
PERL_DEB2 is too obscure for me. Simply use #ifdef DEBUGGING
This removes an extraneous paren pair, and changes the white space in preparation for the next commit
Source code line numbers now have a typedef and format so that they work on a variety of platforms.
This makes the next commits smaller.
They have the same conditional, and the next commit will change that conditional and want it to apply to both.
Some assertions have caused Coverity to complain in the past, or overflowed are too big for some platforms, so the definition of __ASSERT_ in handy.h expands to a no-op if under Coverity or the system doesn't have a normal sized macro buffer. Bring that definition to also be the same for Perl_assert().
And fix up the documentation, adding a caution that their use can easily lead to differing behavior in DEBUGGING vs non-DEBUGGING builds. These two macros are synonymous. This documents assert_() for the first time and clarifies their usage. Thanks to Tony Cook for clarifying this for me.
perl.h
Outdated
| " file \"" __FILE__ "\", line %d", \ | ||
| STRINGIFY(what), __LINE__)) | ||
| " file \"" __FILE__ "\", line %" LINE_Tf, \ | ||
| STRINGIFY(what), (line_t) __LINE__)) |
There was a problem hiding this comment.
This is fine, though line_t is for line numbers stored in COPs (line numbers in perl code) rather than C source.
I doubt it makes a difference unless someone puts #line 4294967296 (__LINE__ is now a long) which changes the code from UB to incorrect output.
(doesn't require a change)
There was a problem hiding this comment.
C99 says #line is UB above 31 bits. Does that affect this?
when was this? I do the Coverity builds with |
Presumably you meant |
perl.h
Outdated
| /* assert() gets defined if DEBUGGING. | ||
| * If no DEBUGGING, the <assert.h> has not been included. */ | ||
| #ifndef assert | ||
| # define assert(what) Perl_assert(what) |
There was a problem hiding this comment.
Commit perl.h: Remove obsolete comment
Hmm... this makes me confused.
The commit message states: assert() is always defined. But the very next line is #ifndef assert, i.e. if not defined assert.
So possibilities I can see:
- the
#ifndef assertis useless and can/should be removed - the statement in the commit message isn't 100% true
| @@ -5096,11 +5096,15 @@ Gid_t getegid (void); | |||
| /* Keep the old croak based assert for those who want it, and as a fallback if | |||
| the platform is so heretically non-ANSI that it can't assert. */ | |||
|
|
|||
| #define Perl_assert(what) PERL_DEB2( \ | |||
There was a problem hiding this comment.
Commit Cleanup Perl_assert definition
I would slightly alter the commit message and add something like:
For reference: PERL_DEB2 is defined as:
#ifdef DEBUGGING
# define PERL_DEB2(a,b) a
#else /* ! DEBUGGING below */
# define PERL_DEB2(a,b) b
#endif /* DEBUGGING */
(I had no clue what PERL_DEB2 did so had to look it up before I could review the change)
perl.h
Outdated
| #ifdef DEBUGGING | ||
| # define Perl_assert(what) \ | ||
| ((what) \ | ||
| ? ((void) 0) \ | ||
| : (Perl_croak_nocontext("Assertion %s failed: file \"" __FILE__ \ |
There was a problem hiding this comment.
Commit Cleanup Perl_assert definition
(using the comment as a scratch-pad to evaluate the change)
Original:
#define Perl_assert(what) PERL_DEB2( \
((what) ? ((void) 0) : \
(Perl_croak_nocontext("Assertion %s failed: file \"" __FILE__ \
"\", line %d", STRINGIFY(what), __LINE__), \
(void) 0)), ((void)0))
Let's make it a bit more readable (at least to me):
#define Perl_assert(what)
PERL_DEB2(
(
(what)
? (
(void) 0
)
: (
Perl_croak_nocontext("Assertion %s failed: file \"" __FILE__ "\", line %d", STRINGIFY(what), __LINE__),
(void) 0
)
),
(
(void)0)
)
That should translate into (ignoring line limits / line continuation markers because they make it just harder to read):
#ifdef DEBUGGING
#define Perl_assert(what)
(
(what)
? (
(void) 0
)
: (
Perl_croak_nocontext("Assertion %s failed: file \"" __FILE__ "\", line %d", STRINGIFY(what), __LINE__),
(void) 0
)
)
#else
#define Perl_assert(what)
(
(void)0)
)
#endif
That almost matches the changed code, the only difference is the else case of the ternary not ending with (void) 0.
I however can't say if that has any significance.
So if that was intended then all is fine.
(Except that it would make me wonder why it was there in the first place, looking at history it appears it was added in commit 2b05728 but again no clue why or if it's needed.)
The combined documentation now cautions about macro parameters being evaluated more than once.
Perl_assert now works correctly on platforms where LINE is not an int., and expands to a no-op under Coverity, or on systems without adequate macro buffer space.