search

sagemath / cysignals

cysignals: interrupt and signal handling for Cython. Source repository for https://pypi.org/project/cysignals/
GNU Lesser General Public License v3.0
43 stars 23 forks source link

Check for macros correctly #138

Closed kliem closed 2 years ago

kliem commented 3 years ago

Many macros are either 1 or undefined.

Instead of

#if CYSIGNALS_ASM_CYSETJMP

we should correctly check

#ifdef CYSIGNALS_ASM_CYSETJMP

See also: https://github.com/sagemath/cysignals/pull/134#discussion_r590435141

collares commented 3 years ago

From the C11 spec:

Preprocessing directives of the forms ``` #if constant-expression new-line group opt #elif constant-expression new-line group opt ``` check whether the controlling constant expression evaluates to nonzero. Prior to evaluation, macro invocations in the list of preprocessing tokens that will become the controlling constant expression are replaced (except for those macro names modified by the `defined` unary operator), just as in normal text. If the token `defined` is generated as a result of this replacement process or use of the `defined` unary operator does not match one of the two specified forms prior to macro replacement, the behavior is undefined. After all replacements due to macro expansion and the `defined` unary operator have been performed, all remaining identifiers (including those lexically identical to keywords) are replaced with the pp-number 0, and then each preprocessing token is converted into a token."

So using #if turns undefined identifiers into 0 by spec.

embray commented 3 years ago

@collares Is that true for older specs, like C99? I was wondering about this as well, since I am usually (if not due to cargo cult) careful to specify #ifdef.

collares commented 3 years ago

The "ISO/IEC 9899:1999, TC3" standard, which is C99 with corrigendas TC1, TC2 and TC3 applied, is available as a draft (N1256) for free. It contains the same quote in page 148 (that's page 160 in a PDF reader).

kliem commented 3 years ago

Tell me, if I should close this. I don't have any strong feelings either way.

collares commented 3 years ago

In my opinion (which doesn't matter, but I was the one being pedantic so I think I should clarify what I meant), clarity is better than exploiting the spec. So if you think #ifdef is clearer, I think this is a worthwhile change even if it isn't necessary. The #ifdef version has the pitfall that people could do #define X 0 expecting to disable X, though.

kliem commented 3 years ago

If we would want to allow #define X 0 we would also have some work to do. I don't expect this to work at the moment either.

Edit: In fact, it doesn't work at the moment. This is why I failed to disable CYSIGNALS_C_ATOMIC in https://trac.sagemath.org/ticket/31245#comment:20. This is a desperate approach of course, but if someone just desperately wants the code work, it makes sense to allow this behavior.

embray commented 3 years ago

It depends on the macro. The HAVE_*_H macros are either defined or not. Likewise any other macros filled in by autoconf are marked #undef by default in the template file, so they are always either undefined or defined to 1. I think #if HAVE_... reads a bit more intuitively though.