On May 9, 2007, at 4:01 PM, Nicholas Clark wrote:
On Wed, May 09, 2007 at 01:06:49PM -0700, chromatic wrote:
On Wednesday 09 May 2007 12:53:57 Nicholas Clark wrote:
On Tue, May 01, 2007 at 04:41:22PM -0700, [EMAIL PROTECTED]
wrote:
+
+#define STRING_IS_NULL(s) ((s) == NULL)
+#define STRING_IS_EMPTY(s) !(int)(s)->strlen
Does !(int)(s)->strlen really scan as quickly and easily as
STRING_IS_EMPTY?
Mmm, yes, thinking about it more...
What's that int cast doing there?
Smells like a bug. Either (s)->strlen is going to be zero, in which
case
! of it is true, or it's going to be in the range INT_MIN to INT_MAX,
in
which case it's not true, or it's going to be outside that range, in
which
case the cast is undefined behaviour. (because it's signed)
Casting to an int is definitely a bug. Since on amd64, int is 32 bits
and INTVAL is 64 bits(and void* is 64 bits), I've become cautious of
any int in the code. But I do find it a tad odd that strlen can be
negative, and I wonder what affect setting it to a negative number
would have on other parts of the system(besides unknown behavior).
Using a macro for anything using casts is probably a decent thing
because of false assumptions about how a compiler chooses the size of a
type. Should maybe add a cage ticket about using things like "int" and
"short" in source files(or anything other than config.h). It'd
hopefully prevent such issues(the sizeof(int) != sizeof(void*) caused a
crash with pccmethods until a refactoring recently) from occuring, at
the cost of commonly used(or assumed) methods for treating data.
I've not checked, and I'm not sure if it's going to be easy to do so,
but
I assume that the cast was moved into the macro as part of refactoring,
and has been in the code for some time.
So, !s->strlen does scan as quickly and easily.
To some, but it isn't as easy to just literally read. "Not s's strlen"
is a lot different than "STRING_IS_EMTPY". Since the code will be read
often, and often by people not familiar with parrot's internals, it
makes sense to make it easily readable. It takes me a second to read
!s->strlen, but half a second to read STRING_IS_EMTPY.
s == NULL is also more tersely written as !s, which, I feel, is also
clearer
to regular C programmers.
Eh, if we have one, may as well have the other, although this one seems
simple enough.
I've also intentionally left the parentheses off, as once you aren't
using
macros, you can choose not to use them on simple expressions.
Arguably one of the mistakes of Perl 5 was to use too many macros,
which
unintentionally contributes to obfuscating the code.
It's not as if *these* are SvPVNL and SvPVZ, or was that SVpvNL or
SvPv
or....?
Yes. Those ones. But after about 5 years I started to see the patterns
in them.
Clearly 5 years isn't a rapid learning curve.
And one of the major reasons I don't want to even look at the perl5
source to find the code I'm wanting. Plus the documentation of the
code isn't great last I saw(like where's the definition of what SvPVNL
is?). Parrot does have a couple flaws for finding struct definitions,
among other things. It took me a couple minutes(and many greps) to
find that STRING is defined in pobj.h as parrot_string_t. Then again,
the macros in question aren't in any string related file but in
interpreter.h(although that one was quicker).
Nicholas Clark
