lutz.euler@... (Lutz Euler) writes:
> The first commit in the patch is to fix an old bug that was introduced
> with a fix for a bug detected by PFD about (RANDOM (IF X 10 20));
> Christophe might remember ...
> Nobody noticed this bug since then (2004). (I hope this means that this
> code path happened to never be used outside of Paul's tests, and not
> that users silently got wrong random distributions.)
> I think the solution to give up the DEFTRANSFORM in this case is OK, but
> would like getting confirmation about this.
Yes, I think so.
The test cases are good; I might try to implement a uniformity test or
two at some point, but the tests in your second commit are clearly
better than no tests at all :)
> Now about the third commit:
> I have put the parts of the new code dealing with random bignums into
> a new file "src/code/random-bignum.lisp". It needs both constants and
> functions from the SB-BIGNUM and the SB-KERNEL package. To put the code
> into the SB-BIGNUM package meant fewer explicit package markers and
> fewer additions to exported symbols. It can't be put into
> "src/code/bignum.lisp" as that comes too early in the build order.
> Is this OK, or should I use "src/code/target-random.lisp" and switch
> packages in the middle of that file?
Although slime can cope with switching packages, it is a bit of a pain
to the (human) reader, and I think the approach you've taken is much
better. Why is src/code/bignum too early, though, what's it using -- do
you remember? If that functionality could be used for anything else it
might be worth calling the file "late-bignum.lisp" rather than
"random-bignum.lisp" -- to make the point that it's code which depends
on something earlier. (If there's no conceivable other use for the
functionality that random-bignum depends on, then fine :-)
> I modified the DEFTRANSFORM but left it in
> "src/compiler/float-tran.lisp". I don't like that very much as the file
> name obviously doesn't fit an integer-only transform. I am not sure
> where to put the DEFTRANSFORM:
> WHN had written in float-tran:
> ;;; FIXME: It's unpleasant to have RANDOM functionality scattered
> ;;; through the code this way. It would be nice to move this into the
> ;;; same file as the other RANDOM definitions.
> RANDOM definitions at that time were in "src/code/random" and
> "src/code/target-random". But, currently nearly all transforms are
> defined in files "compiler/...tran.lisp", and when in 2007 WHN tried to
> improve integer RANDOM he created "src/compiler/integer-tran.lisp" to
> put this transform into (which file, by the way, wasn't deleted when WHN
> retracted his changes, but is now unused).
I have several times been confused by integer-tran.lisp; my memory is
that it actually was removed from CVS, but the cvs->git conversion
didn't pick that up and we've been left with the zombie remains.
There's a fair amount of integer-only functionality in srctran.lisp,
which could also be moved out into an integer-tran (or inttran to
conform to 8.4 file naming restrictions ;-). Initially, modifying the
existing transform is fine, and then maybe reviewing srctran and the
other transform files for integer-only things to be pulled out would be
the way forward.
This looks like a very thorough piece of work. Thank you!