apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Malte S. Stretz" <...@apache.org>
Subject [PATCH] How to Use strcmp to Check for Equality Without Confusing Your Fellow Code, Or: Isn't There a Macro for That?
Date Tue, 19 Oct 2010 16:36:08 GMT
Hi folks,

I recently started to hack on the Apache HTTPD codebase and immediately 
stumbled upon one of my personal code smells:  The usage of strcmp and 
friends and how strings are compared for equality.

I know it is hard to agree on which variant is best, but the Apache 
codebase uses almost all versions I could think of:  Comparing to zero/not 
zero (my preferred version, even though it is a bit confusing), both with 
a pre- and a post-comparison.  And the utterly confusing/hackish negation 
of the result to mean equality; quoting the C FAQ [1]: "It is not 
particularly good style, although it is a popular idiom. The test succeeds 
if the two strings are equal, but the use of ! ("not") suggests that it 
tests for inequality."  There are even some local macros defined in 
mod_ssl.

And there are a lot of string compares in the Apache codebase.  Everytime 
you see a strcmp, you (or is it only me?) have to stop and think "well, is 
this branch checking for equality or the opposite?"

I think this is a case where either a coding standard could help, or some 
helper macros in APR.  I went for the latter and defined APR_EQ plus 
variants in apr_string.h.  See attached patch.

I also wrote a short Perl script to convert all the different strcmp 
invocations to use the new macro(s).  It is attached as well.  Finally I 
did a `find modules -name '*c' | xargs ./apr-str-eq.pl` on the Apache 
trunk and attached the resulting patch so you can judge on your own which 
version you think is easier to grok.

* Why APR_EQ and not something like APR_STREQ?  There are so many string 
compares in Apache that I tried to keep the macro name(s) as short as 
possible.

* Why eg. APR_EQNCASE and not APR_NCASEEQ?  I followed the libc/apr naming 
scheme in the macros but tacked the modifiers to the end because APR_NEQ 
looks like "not equal".

* Why not more underscores, eq. APR_EQ_NCASE?  I don't like it.  But what 
do you think?

* Why not also an APR_NE and friends?  Umm... well, I was lazy 
copy&pasting all those macros.  But I also don't think they're needed.

Comments, flames? :)

Cheers,
Malte

[1]http://c-faq.com/style/strcmp.html

Mime
View raw message