Skip to content

Conversation

@leonerd
Copy link
Contributor

@leonerd leonerd commented Jul 28, 2023

Also a couple of small neatenings of code/test; kept as separate commits in case of subsequent revert

@leonerd
Copy link
Contributor Author

leonerd commented Jul 28, 2023

Hrm; everything passes except the g++ one which fails:

/usr/bin/ld: globals.o:(.data.rel.local+0xd48): undefined reference to `Perl_ck_classname'
/usr/bin/ld: miniperl: hidden symbol `Perl_ck_classname' isn't defined
/usr/bin/ld: final link failed: bad value
collect2: error: ld returned 1 exit status

probably because the newly-added opchecker function isn't done right, somehow. Hrm.

@leonerd
Copy link
Contributor Author

leonerd commented Jul 28, 2023

I can reproduce this locally, and it is fixed by simply moving the Perl_ck_classname() function out of class.c into op.c. Presumably therefore, something isn't expecting to find it in that particular file. Possibly a list of files to expose needs updating? But weird that it only happens under g++ and not regular gcc.

@leonerd
Copy link
Contributor Author

leonerd commented Jul 28, 2023

g++ now fixed. Turns out it needed some extra poking in regen/embed_lib.pl, which @ilmari found. Thanks :)

@leonerd leonerd force-pushed the feature-class branch 2 times, most recently from 14b5eb1 to 0593b62 Compare July 28, 2023 21:17
Comment on lines +1065 to +1066
if(!CvIsMETHOD(PL_compcv))
croak("Cannot use __CLASS__ outside of a method or field initializer expression");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pardon my ignorance 😃 I think ADJUST is currently considered a method, but eventually will become a phaser. It is intended that __CLASS__ will work there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah definitely. The ADJUST blocks will become individual blocks that are all still part of one overall containing method, so __CLASS__ still still work fine there. It'll just stop being one method per block.

Comment on lines +129 to +138
class WithCustomField {
use constant DEFAULT_X => 10;
field $x = __CLASS__->DEFAULT_X;
}

This allows subclasses to override the method with different behaviour.

class DifferentCustomField :isa(WithCustomField) {
sub DEFAULT_X { rand > 0.5 ? 20 : 30 }
}
Copy link
Contributor

@Ovid Ovid Jul 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The initial design of Corinna was to separate methods and subroutines. Thus, you can't call subs like you can call methods. If we had the :common attribute, this could be written as:

    class WithCustomField {
        method DEFAULT_X :common {10}
        field $x = __CLASS__->DEFAULT_X;
    }

This allows subclasses to override the method with different behaviour.

    class DifferentCustomField :isa(WithCustomField) {
        method DEFAULT_X :common { rand > 0.5 ? 20 : 30 }
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed so. This is the best we can write currently, but once we have common methods we can change the documentation to fit the better design.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should add a note about this in the documentation? If people start writing code relying on this, future releases will break it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's all very new and experimental now anyway. Any of the docs should be read in that light anyway, so I don't think adding specific wording to basically every claim and sugestion is going to be all that helpful.

Comment on lines 14 to +26
{
class Test1 {
method hello { return "hello, world"; }

method classname { return __CLASS__; }
}

my $obj = Test1->new;
isa_ok($obj, "Test1", '$obj');

is($obj->hello, "hello, world", '$obj->hello');

is($obj->classname, "Test1", '$obj->classname yields __CLASS__');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test verifying that __CLASS__ is available in ADJUST blocks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added, re-testing now.

@leonerd leonerd added squash-before-merge Author must squash the commits down before merging to blead and removed hasConflicts labels Aug 1, 2023
@leonerd leonerd merged commit 63119cc into Perl:blead Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

squash-before-merge Author must squash the commits down before merging to blead

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants