Steve Hay via RT wrote:
Show quoted text> speaks of different
>behaviour before 5.8.1 compared to 5.8.1 onwards, and yet the original
>code picked out just 5.8.1 for special treatment, which looks odd. Was
>that wrong to start with?
Picking out a single version is pretty much never the right way to
construct a perl version condition. You should almost always compare $]
Show quoted text>= constant (or, for the opposite sense, $] < constant). Of the macros
you have for this, only MP_PERL_VERSION_AT_LEAST() performs this preferred
type of comparison.
In this particular case, it's complicated. 5.8.2 and above, until
the change in 5.17.6, have hash_seed and rehash_seed. 5.8.1 has
only hash_seed. 5.8.0 and below have *neither*, so neither version of
the code there is correct. On those versions the outer conditional,
MP_NEED_HASH_SEED_FIXUP, must be false, which it is by virtue of a
MP_PERL_VERSION_AT_MOST(5, 8, 0) in the logic defining it.
So the existing logic is extensionally correct, in that it produces the
correct result on each Perl version. But it's in a misleading form; it
*looks* wrong. In the course of adding the new branch to the conditional,
with a condition that must be a range check, it seemed least confusing,
albeit strictly unncessary, to reformulate the existing condition into
the standard form. This way the conditions take the same form, they
best express the way changes occur between versions, and the branches
of the conditional appear in strict descending order of version range.
Show quoted text> Should it have said #if
>MP_PERL_VERSION_AT_LEAST(5, 8, 1) rather than #if MP_PERL_VERSION(5, 8, 1)?
No, that would have been incorrect, because that condition
needs to be false on 5.8.2 and above. It could have correctly
been MP_PERL_VERSION_AT_MOST(5, 8, 1). The range check behind
MP_NEED_HASH_SEED_FIXUP could (and ideally should) be expressed
as MP_PERL_VERSION_AT_LEAST(5, 8, 1) rather than the present
!MP_PERL_VERSION_AT_MOST(5, 8, 0).
Show quoted text>In your patched version you now refer to #if MP_PERL_VERSION_AT_LEAST(5,
>8, 2), i.e. 5.8.2 onwards, which is different again. Is that intentional?
This is just doing the (correct) range check the preferred way round.
Show quoted text>Also, am I correct in thinking that the change which this patch is for
>only went into 5.17.6, and was not part of the rehash security fix that
>also went into 5.14.4 and 5.16.3?
Yes. Such a radical change isn't allowed in maint releases these days;
it's not like the 5.8.1 days. 5.16.3 just tweaks the existing hash
mechanism, but 5.17.6 replaces it wholesale.
-zefram
RT for rt.cpan.org