search

xslate / p5-Text-Xslate

Scalable template engine for Perl5
https://metacpan.org/release/Text-Xslate
Other
121 stars 47 forks source link

fix #135 #137

Closed eserte closed 9 years ago

miyagawa commented 9 years ago

This is a meaningless change on the code level to just bypass the warnings - in which case it should better just turn off that specific warning with no warnings.

kazuho commented 9 years ago

Under the premise that we should create a constant in this case, IMO the suggested way is to apply the proposed PR, since that is the suggested of creating a constant according to the updated perldelta.

This usage is now deprecated in those cases where the variable could be modified elsewhere. Perl detects those cases and emits a deprecation warning. Such code will likely change in the future and stop producing a constant. http://search.cpan.org/~bingos/perl-5.21.6/pod/perldelta.pod#Inlining_of_sub_%28%29_{_%24var_}_with_observable_side-effects

miyagawa commented 9 years ago

It's a fairly annoying change on the p5p side IMO, but I agree @kazuho that this is the only way going forward, if this change makes it perl 5.22. (Another way to write it would be make the directory lookup another sub so that my $cache_dir can be initialized in one shot)

However per my discussion with @gfx on twitter, a better fix would be to make the default cache dir location a global variable rather than a constant, since calculating this on a compile time doesn't give any performance benefit such as constant folding.

gfx commented 9 years ago

In this case, as @miyagawa said, I have found better way to avoid this issue (#138), and I'll release a new version today.

IMO this perl's update is useful to write correct code as long as it detects really suspicious one. The problem is that the feature seems still incubating and finds right code as wrong one. It sucks.

eserte commented 9 years ago

@miyagawa: re "meaningless change" --- I think you're wrong here. If the constant was to be retained, then my proposed change is the only way to achieve this. And more or less this is also proposed in perldiag.pod (just try perl -Mdiagnostics ... on that code). no warnings would only help in this perl version, but this construct could be a fatal error in a future one.

miyagawa commented 9 years ago

@eserte i already stated that in the above message.