Combining these two, I have a couple comments.
On Thursday 15 November 2001 02:07 pm, Simon Cozens wrote:
> if (pmc->flags & PS_INTEGER_OK) {
> return ((struct PerlScalarData *)(pmc->data))->intdata;
Why are you storing flags for PerlScalarData inside the pmc-flags? If the
nested type is doing the caching, then it should store the flags.
Additionally, you should probably break out the variables for readibility:
The following assumes macros (bite me), typedefs (don't know why this isn't
standardized, ever since programming in c++, I've never left c-structs
untypedef'd), and unions (as with Uri's suggestion)
static INTVAL Parrot_scalar_get_integer (struct Parrot_Interp *interpreter,
PMC* pmc) {
if ( isPMCScalarClass( pmc ) ) {
PerlScalarData* psd = pmc->data.scalar;
switch ( psd->type ) {
case SCALAR_STR:
psd->intdata = String2Int( psd->stringdata );
setScalarIntOK( psd );
return psd->intdata;
case SCALAR_NUM:
case SCALAR_NUM_STR:
psd->intdata = (int) psd->numdata;
setScalarIntOK( psd );
/* falls through */
case SCALAR_INT_NUM_STR:
case SCALAR_INT_STR:
case SCALAR_INT_NUM:
case SCALAR_INT: return psd->intdata;
default: // throw exception
}
} else {
.... // exception since we're specifically equesting a scalar??
}
}
Nested if-statements could be used instead, but so long as type is <=
num_items, then this gets optimized into a fast table-lookup (with -O2 is
used). Though a bit longer, I think this is still readible.
> On Thu, Nov 15, 2001 at 02:01:53PM -0500, Dan Sugalski wrote:
> > *) Use the cache entry in the PMC header. It'll mean for ints and floats
> > you won't need to bother with a separate structure or dereference
Currently, the "cache" only holds a single value. Is this for space
efficiency? If we broke out the union into independant cached values, then
the two layers could be collapsed into one. (data would go unused for
scalars) Course the switch statement above would have to be masked, as with:
#define getScalarID(x) ( ( x->flags & SCLAR_MASK ) >> SCALAR_MASK_OFFSET )
switch ( getScalarID(pmc) ) {
...
}
In general, I feel that OO-symantics would be most readible here:
if ( pmc->isInteger() ) {
return pmc->getInt();
} else if ( pmc->isNum() ) {
return pmc->cvtNumToInt()->getInt();
} else if ( pmc->isString() ) {
return pmc->cvtStrToInt()->getInt();
}
If this were C++, then the isXXX, cvtXXX would be inlined functions, which
would have the same effect as macro's but with much greater maintainability.
Unfortunately we're using C, so the closest we have is macro's.
While speed is king, readibility is VERY important (as I'm sure we all agree).
The last alternative I can think of is to break PerlScalarData into it's own
sub vtable. Thus
if ( isPMCScalar( pmc ) ) {
return pmc->data.scalar->vtable[ SCALAR_GET_INT ]->( pmc->data.scalar );
}
This would have similar overhead to the table-lookup switch-statement, and
since these are extremely simple functions, many architectures (most notably
SUN) wouldn't require a full blown function call with frame management.
As a last sort of alternative. I had invisioned PMC's has having separate
vtables for each of the many different types of operations. Meaning, that a
scalar-int would be separate from a scalar-string-num, etc. Since it seems
that we're not currently doing this, it would require 8 times as many
vtable's, but it would prevent hardly any if-statements.
>
> I can't. :(
>
> /* It's more efficient to use our own caching than the PMC's cache
> Why? Because we also need to store state on what type of thing is
> in the cache, and this can be one of three things. (four, if you
> include "nothing") Four things is two bits, which means two flag
> tests instead of one. Hence, we just use the old-style IOK/NOK/POK */
>
> > *) Swap out vtables as the pmc internals change, rather than vectoring
> > off flags.
Ops, yeah, that's what I was trying to say above.
>
> That's probably the right way to do it. But what about overlapping types
> that are both integer and string OK?
Hense the larger number of combined types coupled with removing the cache and
just storing each explicit type.
On Thursday 15 November 2001 02:07 pm, Uri Guttman wrote:
> and the PMC would have in it:
>
> union {
>
> INTVAL *int ;
> NUMVAL *num ;
> } data ;
>
-Michael
