On Tue, Mar 02, 2010 at 11:16:49PM -0600, Peter Karman wrote:
> fyi, I've just committed a failing test to KS trunk as r5886. The issue
> manifests when an abstract nullable method is overridden in a subclass. The
> 'nullable' flag is not being parsed correctly in Clownfish and so when the
> overridden method returns undef, KS croaks.
"Croak" is a polite way of putting it, right? I imagine it was segfaulting. :)
> It appears to be a bug in the grammar passed to Parse::RecDescent in
> Clownfish::Parser but that RecDescent stuff is some serious voodoo and I don't
> even know where to start.
Troubleshooting Parse::RecDescent starts with dumping either %item or @item,
the match variables that are created by P::RD for each production. By looking
at %item for the production "type"...
type:
nullable(?) simple_type type_postfix(s?)
{
warn Data::Dumper::Dumper(\%item); # <------ debugging dump
Clownfish::Parser->simple_or_composite_type(\%item)
}
... I was able to see that "nullable" wasn't part of that match:
$VAR1 = {
'__RULE__' => 'type',
'type_postfix(s?)' => [],
'simple_type' => bless( {
'is_string_type' => 0,
'decremented' => 0,
'incremented' => 1,
'c_string' => 'Thing*',
'parcel' => bless( {
'Prefix' => '',
'name' => 'DEFAULT',
'PREFIX' => '',
'prefix' => '',
'cnick' => ''
}, 'Clownfish::Parcel' ),
'const' => undef,
'indirection' => 1,
'specifier' => 'Thing'
}, 'Clownfish::Type::Object' ),
'nullable(?)' => [] # <-------- Missing.
};
Therefore, it had to have been swallowed by the "simple_type" production, which
leads back to "object_type":
simple_type:
object_type # <------- Match here.
| primitive_type
| void_type
| va_list_type
| arbitrary_type
{ $item[1] }
object_type:
type_qualifier(s?) object_type_specifier '*'
{ Clownfish::Parser->new_object_type(\%item); } # <------ calls
constructor
Clownfish::Parser->new_object_type() calls the constructor for
Clownfish::Type::Object, which is ultimately where the problem lay. The
"nullable" parameter was being passed correctly, but the constructor was just
dropping it on the ground.
@@ -30,6 +30,7 @@
$self->{incremented} = $incremented;
$self->{decremented} = $decremented;
$self->{indirection} = $indirection;
+ $self->{nullable} = $nullable;
$self->{parcel} ||= Clownfish::Parcel->default_parcel;
my $prefix = $self->{parcel}->get_prefix;
I committed this change to the KinoSearch repository as r5887; I'll shortly
commit both your test and the fix to the Lucy repository. Thanks for the test!
Marvin Humphrey
