httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Stein <gst...@lyra.org>
Subject Re: cvs commit: apache-2.0/src/lib/apr/network_io/unix networkio.h sockets.c sockopt.c
Date Sat, 16 Oct 1999 20:38:45 GMT
On 16 Oct 1999 rbb@hyperreal.org wrote:
>...
>   1.3       +55 -7     apache-2.0/src/lib/apr/inc/apr_macro.h
>   
>   Index: apr_macro.h
>   ===================================================================
>   RCS file: /home/cvs/apache-2.0/src/lib/apr/inc/apr_macro.h,v
>   retrieving revision 1.2
>   retrieving revision 1.3
>   diff -u -r1.2 -r1.3
>   --- apr_macro.h	1999/10/16 12:47:59	1.2
>   +++ apr_macro.h	1999/10/16 14:06:59	1.3
>   @@ -62,18 +62,20 @@
>    extern "C" {
>    #endif
>    
>   +#include "apr_config.h"
>   +
>    #ifndef _POSIX_THREAD_SAFE_FUNCTIONS
>   -#define SAFETY_LOCK(func_name, name_str) \
>   +#define SAFETY_LOCK(funcname, namestr) \
>        { \
>        if (lock_##func_name == NULL) \
>            if (ap_create_lock(&lock_##func_name, APR_MUTEX, APR_INTRAPROCESS, \

Did you try compiling? It looks like you missed two func_name values here.

>            name_str, NULL) != APR_SUCCESS) \
>                return APR_NOTTHREADSAFE; \
>   -    if (ap_lock(lock_##func_name) != APR_SUCCESS) \
>   +    if (ap_lock(lock_##funcname) != APR_SUCCESS) \
>...
>   @@ -86,15 +88,61 @@
>    #endif
>    
>    #ifdef HAVE_GMTIME_R
>   -#define GMTIME_R(x, y) gmtime_r(x, y)
>   +#define GMTIME_R(x, y) gmtime_r(x, y);
>    #else
>   -#define GMTIME_R(x, y) memcpy(y, gmtime(x), sizeof(y))
>   +#define GMTIME_R(x, y) memcpy(y, gmtime(x), sizeof(y));
>    #endif

Ack!

Ryan, if you're trying to create a macro that has "statement" like
binding, then the above is really not the way to do it. Consider the
following code:

   if (expr)
     GMTIME_R(x,y);
   else
     whatever;

The above will be seriously broken -- there are *two* semicolons in there.
Therefore, the second semicolon terminates the if/else. The compiler will
then see the second "else" and either fail with a syntax error, or (even
worse) bind it to an "if" further up.

There are a couple idioms for doing "statement" macros that I've seen:

#define FOO(x)   if (1) { do_your_stuff(x); } else
#define BAR(x)   do { do_your_stuff(x); } while (0)

Both of these are a proper statement and the semicolon people will use
simply terminates the if/do statement.

>...
>   +#ifdef _POSIX_THREAD_SAFE_FUNCTIONS 
>   +#define GETHOSTBYNAME(x, y, z) z = gethostbyname(x);
>   +#else
>   +#define GETHOSTBYNAME(x, y, z) \
>   +            y = gethostbyname(x); \
>   +            z = ap_palloc(NULL, sizeof(struct hostent)); \
>   +            memcpy(z, y, sizeof(struct hostent));
>   +#endif

The above should be in an if/do so that the statement are properly
combined. The above would fail spectacularly in an if/else where the
person didn't put braces in (like my original example).

>   +#ifdef _POSIX_THREAD_SAFE_FUNCTIONS 
>   +#define GETHOSTBYADDR(x, y, z) z = gethostbyaddr(x, sizeof(struct in_addr), AF_INET);
>   +#else
>   +#define GETHOSTBYADDR(x, y, z) \
>   +            y = gethostbyaddr(x, sizeof(struct in_addr), AF_INET); \
>   +            z = ap_palloc(NULL, sizeof(struct hostent)); \
>   +            memcpy(z, y, sizeof(struct hostent));
>   +#endif

if/do here, too.

>   +#ifdef _POSIX_THREAD_SAFE_FUNCTIONS 
>   +#define INET_NTOA(x, y, len) ap_cpystrn(y, inet_ntoa(x), len);
>   +#else
>   +#define INET_NTOA(x, y, len) ap_cpystrn(y, inet_ntoa(x), len);
>   +#endif

Why are these two conditions the same? Do you imagine they would be
different at some point? Did you just skip writing the full logic for this
one now, to come back to it later?

>   +#ifdef _POSIX_THREAD_SAFE_FUNCTIONS 
>   +#define READDIR(x, y, z)  y = readdir(x); \
>   +                           if (y == NULL) { \
>   +                               z = errno; \
>   +                           } \
>   +                           else { \
>   +                               z = APR_SUCCESS; \
>   +                           }
>    #else
>   -#define LOCALTIME_R(x, y) memcpy(y, localtime(x), sizeof(y))
>   +#define READDIR(x, y, z) \
>   +                          { \
>   +                          struct dirent *temp = readdir(x); \
>   +                          if (temp == NULL) { \
>   +                              z = errno; \
>   +                          } \
>   +                          else { \
>   +                              memcpy(y, temp, sizeof(struct dirent)); \
>   +                              z = APR_SUCCESS; \
>   +                          } \
>   +                          }

Definitely needs an if/do to properly scope this stuff.

Cheers,
-g

--
Greg Stein, http://www.lyra.org/


Mime
View raw message