Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#11854 closed defect (fixed)

Fix conversion QQ -> PARI

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

Description (last modified by jdemeyer)

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 (1)

11854.patch​ (20.3 KB) - added by jdemeyer 8 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 8 years ago by jdemeyer

  • Dependencies set to #11130

comment:2 Changed 8 years ago by jdemeyer

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

comment:3 Changed 8 years ago by jdemeyer

  • Status changed from new to needs_review

comment:4 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-4.7.2 to sage-4.7.3

comment:5 Changed 8 years ago by jdemeyer

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

comment:6 Changed 8 years ago by jdemeyer

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

comment:7 follow-up: Changed 8 years 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 8 years ago by jdemeyer

comment:8 in reply to: ↑ 7 Changed 8 years 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 8 years ago by jdemeyer

  • Reviewers set to William Stein

comment:10 Changed 8 years ago by jdemeyer

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

comment:11 Changed 8 years ago by jdemeyer

  • Milestone sage-4.7.3 deleted

Milestone sage-4.7.3 deleted

comment:12 Changed 8 years 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.