Reformulating a macro to use argument just once

[Francois Grieu]

Consider this macro

// check if x, assumed of type unsigned char, is in range [0x20..0x7E]
#define ISVALID(x) ((x)>=0x20 && (x)<=0x7E)

Of course, this can't be safely used as in
if (ISVALID(*p++)) foo();
where p is a pointer ot unsigned char.


Unless I err, this issue can be fixed (and often, performance
improved) using

#define ISVALID(x) ((unsigned char)((x)-0x20)<=0x7E-0x20)



Buf can we do something similar about this one?

// check if x, assumed of type unsigned char,
// is in range [0x20..0x7E] or at least 0xC0
#define ISVALID(x) ((x)>=0x20 && (x)<=0x7E || (x)>=0xC0)



Francois Grieu
[reposted with a fix in the last comment]

 

[loic-dev]

Bonjour Francois,

Consider this macro

// check if x, assumed of type unsigned char, is in range [0x20..0x7E]
#define ISVALID(x) ((x)>=0x20 && (x)<=0x7E)

Of course, this can't be safely used as in
if (ISVALID(*p++)) foo();
where p is a pointer ot unsigned char.


Unless I err, this issue can be fixed (and often, performance
improved) using

#define ISVALID(x) ((unsigned char)((x)-0x20)<=0x7E-0x20)



Buf can we do something similar about this one?

// check if x, assumed of type unsigned char,
// is in range [0x20..0x7E] or at least 0xC0
#define ISVALID(x) ((x)>=0x20 && (x)<=0x7E || (x)>=0xC0)

Two ideas that come to my mind:

1) declare ISVALID as (inlined) function, or

2) use an additional temporary global variable:

unsigned char tmp_var;
#define ISVALID(x) ((tmp_var=x)>=0x20 && (tmp_var)<=0x7E ||
(tmp_var)>=0xC0)


Personaly I'd go for 1) if your compiler support inlined function.

HTH,
Loic.

 

[Guest]

Consider this macro

// check if x, assumed of type unsigned char, is in range [0x20..0x7E]
#define ISVALID(x) ((x)>=0x20 && (x)<=0x7E)

Of course, this can't be safely used as in
if (ISVALID(*p++)) foo();
where p is a pointer ot unsigned char.


Unless I err, this issue can be fixed (and often, performance
improved) using

#define ISVALID(x) ((unsigned char)((x)-0x20)<=0x7E-0x20)



Buf can we do something similar about this one?

// check if x, assumed of type unsigned char,
// is in range [0x20..0x7E] or at least 0xC0
#define ISVALID(x) ((x)>=0x20 && (x)<=0x7E || (x)>=0xC0)

You probably really shouldn't be using a macro here.

That aside, you can use a table. Assuming your values are never more
than 255, it would look like

char valid[256] = {
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1
};

#define ISVALID(x) (valid[(x)])

 

[Arthur J. O'Dwyer]

Consider this macro

// check if x, assumed of type unsigned char, is in range [0x20..0x7E]
#define ISVALID(x) ((x)>=0x20 && (x)<=0x7E)

Of course, this can't be safely used as in
if (ISVALID(*p++)) foo();
where p is a pointer ot unsigned char.

Unless I err, this issue can be fixed (and often, performance
improved) using

#define ISVALID(x) ((unsigned char)((x)-0x20)<=0x7E-0x20)

Ick! Don't forget the long comment explaining what effect this
weird contortion has. Once you account for those two or three lines,
you may find it more efficient simply to add a /shorter/ comment
explaining that you shouldn't pass arguments with side effects to
macros.

Buf can we do something similar about this one?

// check if x, assumed of type unsigned char,
// is in range [0x20..0x7E] or at least 0xC0
#define ISVALID(x) ((x)>=0x20 && (x)<=0x7E || (x)>=0xC0)

You probably really shouldn't be using a macro here.

IMHO you should, but you *really* shouldn't be passing it an argument
with a side effect. In fact, with the exception of "nesting" calls to
true functions, like 'foo(bar(x))', IMO you shouldn't put side effects
inside parentheses[1] no matter whether they're part of a macro or not!

That aside, you can use a table. Assuming your values are never more
than 255, it would look like

char valid[256] = {
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,

Typo in this line, unfortunately. Should be

1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0,

If it weren't for that fact, then the entire table could
be collapsed down into eight entries and a mask. But since
0x7F is being excluded, the collapsing doesn't work so
nicely.

0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1
};

#define ISVALID(x) (valid[(x)])

The inner parentheses aren't necessary there,[2] and I would omit them.
(The outer ones aren't necessary either, but I'd keep them for the
benefit of less clueful maintainers in the future. IMHO the inner
parentheses harm readability, and the outer ones don't have any
effect one way or the other on readability.)

-Arthur

[1] - With the exception of for (i=0; i < n; ++i), of course.
But MYMACRO(i=42) is right out, as is assert(n++ < 255), thus
saving us from a particularly nasty class of bugs. And forswearing
while (*s++ = *--t) means slightly less time figuring out what
your code does, when you look at it next month.

[2] - Assuming you trust yourself not to write ISVALID(0][0) or
any such syntax-perverting nonsense. And I do.

 

[Eric Sosman]

Consider this macro

// check if x, assumed of type unsigned char, is in range [0x20..0x7E]
#define ISVALID(x) ((x)>=0x20 && (x)<=0x7E)

Of course, this can't be safely used as in
if (ISVALID(*p++)) foo();
where p is a pointer ot unsigned char.

Unless I err, this issue can be fixed (and often, performance
improved) using

#define ISVALID(x) ((unsigned char)((x)-0x20)<=0x7E-0x20)

Ick! Don't forget the long comment explaining what effect this
weird contortion has. Once you account for those two or three lines,
you may find it more efficient simply to add a /shorter/ comment
explaining that you shouldn't pass arguments with side effects to
macros.

It's not a "weird contortion," but a common and useful idiom.
Yes, it relies on overflow behaviors that C does not guarantee,
but that's easily fixable:

#define ISVALID(x) ( \
(unsigned char)(x) - 0x20u <= 0x7Eu - 0x20u )

(The second and third `u' suffixes could be omitted safely.)

 

[Guest]

Consider this macro

// check if x, assumed of type unsigned char, is in range [0x20..0x7E]
#define ISVALID(x) ((x)>=0x20 && (x)<=0x7E)

Of course, this can't be safely used as in
if (ISVALID(*p++)) foo();
where p is a pointer ot unsigned char.

Unless I err, this issue can be fixed (and often, performance
improved) using

#define ISVALID(x) ((unsigned char)((x)-0x20)<=0x7E-0x20)

Ick! Don't forget the long comment explaining what effect this
weird contortion has. Once you account for those two or three lines,
you may find it more efficient simply to add a /shorter/ comment
explaining that you shouldn't pass arguments with side effects to
macros.

Buf can we do something similar about this one?

// check if x, assumed of type unsigned char,
// is in range [0x20..0x7E] or at least 0xC0
#define ISVALID(x) ((x)>=0x20 && (x)<=0x7E || (x)>=0xC0)

You probably really shouldn't be using a macro here.

IMHO you should, but you *really* shouldn't be passing it an argument
with a side effect.

Okay. Would you disagree with "If you're going to pass it an
argument with a side effect, you probably really shouldn't be using a
macro here." ?

In fact, with the exception of "nesting" calls to
true functions, like 'foo(bar(x))', IMO you shouldn't put side effects
inside parentheses[1] no matter whether they're part of a macro or not!

And in my opinion, if you use function-like macros, you should write
them in such a way that the user does not need to care whether it's
implemented as a macro or as a function. Which makes foo(bar(x)) okay
even if foo is a macro.

That aside, you can use a table. Assuming your values are never more
than 255, it would look like

char valid[256] = {
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,

Typo in this line, unfortunately. Should be

1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0,

Oops, quite right.

If it weren't for that fact, then the entire table could
be collapsed down into eight entries and a mask. But since
0x7F is being excluded, the collapsing doesn't work so
nicely.

0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1
};

#define ISVALID(x) (valid[(x)])

The inner parentheses aren't necessary there,[2] and I would omit them.
(The outer ones aren't necessary either, but I'd keep them for the
benefit of less clueful maintainers in the future. IMHO the inner
parentheses harm readability, and the outer ones don't have any
effect one way or the other on readability.)

I would keep both pairs, for the same reason. I have got into the habit
of always parenthesising macro arguments, and the whole macro
expansion, without checking whether they're strictly necessary. If you
feel the inner parentheses hurt readability, some spacing should solve
that.

-Arthur

[1] - With the exception of for (i=0; i < n; ++i), of course.
But MYMACRO(i=42) is right out, as is assert(n++ < 255), thus
saving us from a particularly nasty class of bugs.

What about if(n++ < 255) ?

And forswearing
while (*s++ = *--t) means slightly less time figuring out what
your code does, when you look at it next month.

That one's easy enough to figure out, especially if you're familiar
with the common while(*s++ = *t++). I wouldn't write it like that
either, though.

[2] - Assuming you trust yourself not to write ISVALID(0][0) or
any such syntax-perverting nonsense. And I do.

Heh, I didn't think of that. Yeah, that should be a safe assumption.

 

[Ben Pfaff]

// check if x, assumed of type unsigned char,
// is in range [0x20..0x7E] or at least 0xC0
#define ISVALID(x) ((x)>=0x20 && (x)<=0x7E || (x)>=0xC0)

You probably really shouldn't be using a macro here.

I suppose you could do this:
#define ISVALID_MACRO(x) ((x)>=0x20 && (x)<=0x7E || (x)>=0xC0)
static int is_valid(int x) { return ISVALID_MACRO(x); }
and get the best of both worlds (in some sense), although in
practice I'd only use a macro here if profiling showed it to be a
win.

 

[Francois Grieu]

Francois Grieu wrote:
Consider this macro

// check if x, assumed of type unsigned char, is in range [0x20..0x7E]
#define ISVALID(x) ((x)>=0x20 && (x)<=0x7E)

Of course, this can't be safely used as in
if (ISVALID(*p++)) foo();
where p is a pointer ot unsigned char.

Unless I err, this issue can be fixed (and often, performance
improved) using

#define ISVALID(x) ((unsigned char)((x)-0x20)<=0x7E-0x20)

Ick! Don't forget the long comment explaining what effect this
weird contortion has. Once you account for those two or three lines,
you may find it more efficient simply to add a /shorter/ comment
explaining that you shouldn't pass arguments with side effects to
macros.

It's not a "weird contortion," but a common and useful idiom.

Yes ! Thanks for the appreciation.

Yes, it relies on overflow behaviors that C does not guarantee,

Yes, I silently assumed that UCHAR_MAX = 255. But all the rest
follows from 6.3.1.3 clause 2:
"if the new type is unsigned, the value is converted by
repeatedly adding or substracting one more that the maximum
value that can be represented in the new type until the
value is in the range of the new type."

but that's easily fixable:

#define ISVALID(x) ( \
(unsigned char)(x) - 0x20u <= 0x7Eu - 0x20u )

(The second and third `u' suffixes could be omitted safely.)

Indeed this should work, since (unsigned char)(x) - 0x20u
must be unsigned and calculated modulo (UINT_MAX+1).
Even the cast is unnecessary if x is assumed to be of type
unsigned char (that was stated).

However on 8-bit embedded compilers that I practice, it still helps
to cast the result of (x) - 0x20u to type unsigned char, else
the compiler performs two-bytes arithmetic.


François Grieu

 

[Arthur J. O'Dwyer]

Francois Grieu wrote:
Consider this macro

// check if x, assumed of type unsigned char, is in range [0x20..0x7E]
#define ISVALID(x) ((x)>=0x20 && (x)<=0x7E)

Of course, this can't be safely used as in
if (ISVALID(*p++)) foo();
where p is a pointer ot unsigned char.

Unless I err, this issue can be fixed (and often, performance
improved) using

#define ISVALID(x) ((unsigned char)((x)-0x20)<=0x7E-0x20)

Ick! Don't forget the long comment explaining what effect this
weird contortion has.

(For the record, and FWIW, I stand by my characterization of that
line of code as a "weird contortion". It's a contorted way of writing
what Francois /really/ meant, and serves no purpose except as a way
to hack in the ability to pass side-effectful arguments to the macro.
Whether or not it's standard C has no bearing on its weirdness or its
contortedness.)

Okay. Would you disagree with "If you're going to pass it an
argument with a side effect, you probably really shouldn't be using a
macro here." ?

Sure. Vacuous implication, and all that.

WARNING: The rest of this post is merely good-natured blithering
about coding style, and should not be considered grounds for a
long religious war!

In fact, with the exception of "nesting" calls to
true functions, like 'foo(bar(x))', IMO you shouldn't put side effects
inside parentheses[1] no matter whether they're part of a macro or not!

And in my opinion, if you use function-like macros, you should write
them in such a way that the user does not need to care whether it's
implemented as a macro or as a function. Which makes foo(bar(x)) okay
even if foo is a macro.

If foo is a macro, it should be spelled FOO, and the user should not
be passing it any arguments with side-effects. Although I certainly
agree that macros should not /gratuitously/ evaluate their arguments
more than once, in any event.

#define ISVALID(x) (valid[(x)])

The inner parentheses aren't necessary there,[2] and I would omit them.
(The outer ones aren't necessary either, but I'd keep them for the
benefit of less clueful maintainers in the future. IMHO the inner
parentheses harm readability, and the outer ones don't have any
effect one way or the other on readability.)

I would keep both pairs, for the same reason. I have got into the habit
of always parenthesising macro arguments, and the whole macro
expansion, without checking whether they're strictly necessary.

Yup. I do the same, but my habit excludes parenthesizing arguments
that are /already/ inside bracket-like structures: foo(x) instead of
foo((x)); foo(x,y) instead of foo((x),(y)); and (a) instead of
(a)[(i)].

If you feel the inner parentheses hurt readability, some spacing
should solve that.

Nope, I also detest extra spacing.

[1] - With the exception of for (i=0; i < n; ++i), of course.
But MYMACRO(i=42) is right out, as is assert(n++ < 255), thus
saving us from a particularly nasty class of bugs.

What about if(n++ < 255) ?

Nope, that's no good. I'd write

if (n < 255) {
n += 1;
...
} else {
n += 1;
...
}

or, more likely, change the type of 'n' so that the addition could be
performed earlier:

n += 1;
if (n < 256) {
...
}

That one's easy enough to figure out, especially if you're familiar
with the common while(*s++ = *t++). I wouldn't write it like that
either, though.

Yep, that's my point. It's "easy enough" to figure out for any
competent C programmer, but it's even easier to figure out if it's
not there at all.

my $.02,
-Arthur

 

[Eric Sosman]

Consider this macro
[...]
#define ISVALID(x) ((unsigned char)((x)-0x20)<=0x7E-0x20)

Yes, it relies on overflow behaviors that C does not guarantee,

Yes, I silently assumed that UCHAR_MAX = 255. But all the rest
follows from 6.3.1.3 clause 2:
"if the new type is unsigned, the value is converted by
repeatedly adding or substracting one more that the maximum
value that can be represented in the new type until the
value is in the range of the new type."

That isn't the problem I was thinking of. In the original,
all three hex constants are of type int, a.k.a. signed int. If
x is also an int or is of a narrower type that promotes to int,
the subtraction (x)-0x20 is carried out in int arithmetic. For
a "narrow" x this is probably all right, but for an int x with a
large negative value the subtraction could overflow -- and when
that happens, you've got undefined behavior. The conversion to
unsigned char comes too late.

Indeed this should work, since (unsigned char)(x) - 0x20u
must be unsigned and calculated modulo (UINT_MAX+1).
Even the cast is unnecessary if x is assumed to be of type
unsigned char (that was stated).

Sorry; I'd overlooked the assumption about x's type. In
that case, the original macro is all right: Even if unsigned
char promotes to int, the promoted value will be non-negative
and the subtraction will not overflow. Still, I'd suggest the
revised macro as slightly more "defensive" against programmers
who might supply x'es of unexpected type.