Number_types: fix Lazy_exact_nt operator<< to write exact() value#9431
Number_types: fix Lazy_exact_nt operator<< to write exact() value#9431RajdeepKushwaha5 wants to merge 2 commits intoCGAL:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes Lazy_exact_nt<ET> stream output so that saving/loading exact values round-trips without precision loss, addressing issue #135 in the Number_types package.
Changes:
- Update
operator<<forLazy_exact_nt<ET>to writea.exact()instead ofto_double(a). - Update documentation to describe the new exact output behavior.
- Add and register a regression test that exercises round-trip I/O for several rational/integer cases.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| Number_types/include/CGAL/Lazy_exact_nt.h | Changes streaming output to use the exact value (exact()), enabling lossless round-trip with existing operator>>. |
| Number_types/doc/Number_types/CGAL/Lazy_exact_nt.h | Updates docs to match the new exact-output semantics and round-trip intent. |
| Number_types/test/Number_types/Lazy_exact_nt_io.cpp | Adds regression coverage for round-trip I/O of exact rationals/integers (including large values). |
| Number_types/test/Number_types/CMakeLists.txt | Registers the new single-source test program. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| std::istringstream iss(oss.str()); | ||
| Lazy_nt b; | ||
| iss >> b; | ||
| assert(a == b); |
There was a problem hiding this comment.
Same issue as above: the test should assert the stream extraction succeeded after iss >> b to avoid false positives if parsing fails.
| std::istringstream iss(oss.str()); | ||
| Lazy_nt b; | ||
| iss >> b; | ||
| assert(a == b); |
There was a problem hiding this comment.
Same issue as above: please assert the stream is still good after iss >> b so the test can’t pass if parsing fails.
| oss << c; | ||
| std::istringstream iss(oss.str()); | ||
| Lazy_nt d; | ||
| iss >> d; |
There was a problem hiding this comment.
Same issue as above: add a stream-state assertion after iss >> d to ensure the read succeeded before comparing c == d.
| iss >> d; | |
| iss >> d; | |
| assert(iss); |
| std::istringstream iss(oss.str()); | ||
| Lazy_nt b; | ||
| iss >> b; | ||
| assert(a == b); |
There was a problem hiding this comment.
Same issue as above: assert successful extraction after iss >> b so the test can’t pass if parsing fails.
| oss << a; | ||
| std::istringstream iss(oss.str()); | ||
| Lazy_nt b; | ||
| iss >> b; |
There was a problem hiding this comment.
Same issue as above: because the expected value is 0 here, a failed parse could leave b at the default value and make the assertion pass. Add an assert(iss) / assert(!iss.fail()) after iss >> b.
| iss >> b; | |
| iss >> b; | |
| assert(iss); |
| std::istringstream iss(oss.str()); | ||
| Lazy_nt b; | ||
| iss >> b; | ||
| assert(a == b); |
There was a problem hiding this comment.
The test doesn’t verify that extraction succeeded (stream state) before comparing values. If parsing fails, b can remain at its default value (which is 0 for Lazy_exact_nt), potentially making the test pass spuriously (notably in the zero case). Add an assertion like assert(iss) / assert(!iss.fail()) after iss >> b (and similarly in the other test blocks).
Change operator<< for Lazy_exact_nt to output a.exact() instead of to_double(a). The old to_double() conversion loses precision and breaks round-trip save/load of exact kernel coordinates (issue CGAL#135). The corresponding operator>> already uses read_float_or_quotient() which handles both floating-point and rational (n/d) formats, so no changes are needed on the input side. Add Lazy_exact_nt_io.cpp regression test verifying round-trip I/O for rationals, integers, negative values, computed sums, large values, and zero.
56d312f to
a935007
Compare
…stream state Addresses review feedback on PR 9431: - Replace CGAL::Gmpq/Gmpz with CGAL::Exact_rational so the test compiles without GMP (per afabri's comment). - Add assert(iss) after every stream extraction to fail loudly on parse errors (per Copilot AI review).
Description
Summary of Changes
operator<<forLazy_exact_nt<ET>was usingto_double(a), which loses precision and breaks round-trip save/load of exact kernel coordinates. For example, writingLazy_exact_nt<Gmpq>(1/3)would output0.333333instead of1/3, and reading it back would produce a different exact value.This PR changes
operator<<to outputa.exact()instead, so thatoperator>>(which already usesread_float_or_quotient()and handles both floating-point and rationaln/dformats) can reconstruct the same exact number.Changes
Number_types/include/CGAL/Lazy_exact_nt.h: Changedoperator<<fromos << CGAL_NTS to_double(a)toos << a.exact()Number_types/doc/Number_types/CGAL/Lazy_exact_nt.h: Updated documentation to reflect the new exact output formatNumber_types/test/Number_types/Lazy_exact_nt_io.cpp: Added round-trip I/O regression test covering rationals (1/3), integers (42), negatives (-5/7), computed sums (10/21), large values (99999999999999999999/100000000000000000007), and zeroNumber_types/test/Number_types/CMakeLists.txt: Registered the new testRelease Management