Re: [perl #128966] Bleadperl v5.25.3-232-g10f9b9b breaksLEONT/Const-Fast-0.014.tar.gz

On 24 August 2016 at 16:41, Leon Timmermans <fawaka@gmail.com> wrote:
> On Tue, Aug 16, 2016 at 9:44 PM, Andreas J. Koenig via RT
> <perlbug-followup@perl.org> wrote:
>>
>> bisect
>> ------
>> commit 10f9b9bf77ed09ee67da058d1ba1658a4ce67626
>> Author: Yves Orton <demerphq@gmail.com>
>> Date:   Mon Aug 8 18:53:20 2016 +0200
>>
>>     move Internals::hv_clear_placeholders() to
>> Hash::Util::_clear_placeholders()
>>
>> diagnostics
>> -----------
>>
>> http://www.cpantesters.org/cpan/report/40ee7106-62a0-11e6-b236-c19558b9f28c
>
>
> I think the removal from Internals:: was a bit premature.

TL;DR: It was readded some time ago. But if you are going to use
undocumented internals features and unroll official functionality to
reimplement or extend that functionality then IMO you get to keep the
pieces when the internals changes.

> This function is
> useful outside of Hash::Util in one particular circumstance: when making a
> hash readonly (or actually restricted). Moving this but leaving SvREADONLY
> is suboptimal, what you really want is make_const function that takes care
> of this if it's a hash.

I have a few issues with this.

First off, Internals::hv_clear_placehold() is only needed to get rid
of placeholders stored in an *ALREADY* restricted hash.

So, if your code is meant to mark a data structure readonly then it is
not needed.

Second, as far as I can tell the code in Const::Fast overlaps with and
extends Hash::Util, and as the official place to expose
readonly/restricted hashes IMO you should have either used Hash::Util,
or patched Hash::Util to offer a way to recursively handle other types
or to perhaps expose hv_clear_placeholders() via Hash::Util so you
could use it as a documented function.

However what you have done in this module is use undocumented
internals routines to unroll the official functionality. In theory as
undocumented internals functions we should be able to remove or change
Internals::hv_clear_placehold() and Internals::SvREADONLY() in any way
we choose, like I did, and not break any well written code. But you
have decided to violate the encapsulation and as far as I am concerned
when you do that you get to keep both pieces. (Like I do in
Data::Dump::Streamer or Sereal when something internals changes.)

Third, I think that functionality like Const::Fast should be coded in
XS, and be part of the core. If its meant to be fast then it should be
XS, and since it didles internals flags that have uncertain and
ambiguous meanings (such as the restricted/readonly ambiguity) it
should probably be part of core and maintained as part of core.
Essentially you are using undocumented functions to fill a
functionalitry vacuum that core has not filled. I think it would be
better for all if this functionality lived in core.

Having said all that: I re-added hv_clear_placehold() back to
Internals some months back. I personally think we should mark the
Internals functions deprecated and make them throw deprecation
warnings and require people to use well defined and well documented
functions to achieve the same results.

>
> Or actually, what would be even better is having a sensible split between
> readonly and restricted hashes, but that's a different discussion.

I think we should ditch restricted hashes, and just have what you
would call readonly hashes. But as you say that is a different
discussion.

Yves


-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

• Re: [perl #128966] Bleadperl v5.25.3-232-g10f9b9b breaksLEONT/Const-Fast-0.014.tar.gz by demerphq