Skip to content

Support for Class::XSAccessor accessors - #26

Open
tobyink wants to merge 3 commits into
dagolden:masterfrom
tobyink:master
Open

tobyink wants to merge 3 commits into
dagolden:masterfrom
tobyink:master

Conversation

@tobyink

@tobyink tobyink commented May 15, 2018

Copy link
Copy Markdown
Contributor

This six line addition roughly doubles the speed of any accessors that don't have a default by building them with Class::XSAccessor. It has a negligible effect on start up time and memory usage. When Class::XSAccessor is not installed, it has no effect. No extra test cases are provided because the intention is for this to have no detectable effect other than the speed-up.

Possible additional changes: adding Class::XSAccessor as a suggests dependency.

@coveralls

coveralls commented May 15, 2018

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-1.5%) to 97.015% when pulling 154e887 on tobyink:master into e19ae20 on dagolden:master.

@tobyink

tobyink commented May 21, 2018

Copy link
Copy Markdown
Contributor Author

I don't know whether you consider the coverage thing to be a problem. Using Devel::Hide, or maybe an environment variable toggle, it might be possible to test both the XS and PP routes in the same test run. Would you like me to try to add something like that to the test suite and .travis.yml?

@xdg

xdg commented May 21, 2018 via email

Copy link
Copy Markdown
Contributor

@xdg

xdg commented Jun 20, 2018

Copy link
Copy Markdown
Contributor

Something that I'd like to figure out before merging this is how to test with/without Class::XSAccessors.

Also need to ensure the prereq is stripped back out in the dist.ini.

@tobyink

tobyink commented Jun 21, 2018

Copy link
Copy Markdown
Contributor Author

Could do something like:

my $HAS_XS = !$ENV{PERL_CLASS_TINY_PP}
    && eval { require Class::XSAccessor; 1 };

And write a few test cases which do this right at the top before loading Class::Tiny:

BEGIN { $ENV{PERL_CLASS_TINY_PP} = 1 };

As far as declaring Class::XSAccessor as an optional dependency for Dist::Zilla, I don't know. I don't use DIst::Zilla myself, but I'm certain it can be done.

@xdg

xdg commented Jun 21, 2018

Copy link
Copy Markdown
Contributor

I can do the dzil work, that was more a comment to self.

@tobyink

tobyink commented Jul 3, 2018

Copy link
Copy Markdown
Contributor Author

Do you think the tests I submitted are sufficient?

@xdg

xdg commented Jul 3, 2018

Copy link
Copy Markdown
Contributor

Sorry, haven't had a chance to look. I'm a bit underwater with $work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants