Ticket #11854 (closed defect: fixed)

Opened 20 months ago

Last modified 19 months ago

Fix conversion QQ -> PARI

Reported by: jdemeyer Owned by: was
Priority: major Milestone: sage-4.8
Component: interfaces Keywords:
Cc: Work issues:
Report Upstream: N/A Reviewers: William Stein
Authors: Jeroen Demeyer Merged in: sage-4.8.alpha1
Dependencies: #11611, #11685 Stopgaps:

Description (last modified by jdemeyer) (diff)

Same problem as #11611 but for QQ instead of ZZ, the hash of an element depends on previous garbage on the PARI stack. This is because the conversion QQ->PARI is not done properly. 

sage: foo = pari(2^(32*1024));
sage: hash(pari(QQ(1)))
-8646911284551352313
sage: foo = pari(0xDEADBEEF * (2^(32*1024)-1)//(2^32 - 1));
sage: hash(pari(QQ(1)))
-5476377146882523129

Attachments

11854.patch Download (20.3 KB) - added by jdemeyer 19 months ago.

Change History

comment:1 Changed 20 months ago by jdemeyer

  • Dependencies set to #11130

comment:2 Changed 20 months ago by jdemeyer

  • Dependencies changed from #11130 to #11130, #11685

comment:3 Changed 20 months ago by jdemeyer

  • Status changed from new to needs_review

comment:4 Changed 20 months ago by jdemeyer

  • Milestone changed from sage-4.7.2 to sage-4.7.3

comment:5 Changed 20 months ago by jdemeyer

  • Dependencies changed from #11130, #11685 to #11685
  • Authors set to Jeroen Demeyer

comment:6 Changed 20 months ago by jdemeyer

  • Dependencies changed from #11685 to #11611, #11685
  • Description modified (diff)

comment:7 follow-up: ↓ 8 Changed 19 months ago by was

  • Status changed from needs_review to positive_review

The code looks excellent and this is an extremely important patch. (An English expert may point out that "For internal use only, this directly uses the PARI stack." is a run-on sentence, and should be "For internal use only; this directly uses the PARI stack.")

Also all tests pass, so positive review.

Changed 19 months ago by jdemeyer

comment:8 in reply to: ↑ 7 Changed 19 months ago by jdemeyer

Replying to was:

An English expert may point out that "For internal use only, this directly uses the PARI stack." is a run-on sentence, and should be "For internal use only; this directly uses the PARI stack."

Fixed this.

Many thanks for the review!

comment:9 Changed 19 months ago by jdemeyer

  • Reviewers set to William Stein

comment:10 Changed 19 months ago by jdemeyer

  • Status changed from positive_review to closed
  • Resolution set to fixed
  • Merged in set to sage-4.7.3.alpha1

comment:11 Changed 19 months ago by jdemeyer

  • Milestone sage-4.7.3 deleted

Milestone sage-4.7.3 deleted

comment:12 Changed 19 months ago by jdemeyer

  • Merged in changed from sage-4.7.3.alpha1 to sage-4.8.alpha1
  • Milestone set to sage-4.8
Note: See TracTickets for help on using tickets.