2026 PKUCourseHW5: more precise log for Fcoef::create#7126
2026 PKUCourseHW5: more precise log for Fcoef::create#7126kevin0x3f wants to merge 1 commit intodeepmodeling:developfrom
Conversation
kevin0x3f
changed the title
There was a problem hiding this comment.
Pull request overview
This PR improves the diagnostic output from Fcoef::create by making the log messages more precise and adding extra context when invalid inputs are detected.
Changes:
- Prefixes error logs with
Fcoef::createfor clearer source attribution. - Adds a “possible integer overflow” diagnostic that logs
i1,i2,i3, and their computed product.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!(i1 * i2 * i3 > 0)){ | ||
| std::cout << "i1*i2*i3 must be positive! i1*i2*i3 is " << i1 * i2 * i3 << std::endl; | ||
| unsigned long long product = static_cast<unsigned long long>(i1) * static_cast<unsigned long long>(i2) * static_cast<unsigned long long>(i3); | ||
| std::cout << "Fcoef::create: possible integer overflow in product i1*i2*i3! i1 is " << i1 << ", i2 is " << i2 << ", i3 is " << i3 << ", product is " << product << std::endl; | ||
| } |
There was a problem hiding this comment.
The condition i1 * i2 * i3 > 0 can itself trigger signed integer overflow (undefined behavior) before you get to the diagnostic, which defeats the purpose of detecting overflow. Also, casting negative int values to unsigned long long will produce very large values, so the printed product can be misleading when the real problem is simply a negative input. A safer approach is to check i1/i2/i3 positivity without multiplying, and only compute a product using a wide signed type (e.g., long long/__int128) after confirming all are positive (or perform overflow-checked multiplication).
| std::cout << "Fcoef::create: not allowed!" << std::endl; | ||
| if (!(i1 * i2 * i3 > 0)){ | ||
| std::cout << "i1*i2*i3 must be positive! i1*i2*i3 is " << i1 * i2 * i3 << std::endl; | ||
| unsigned long long product = static_cast<unsigned long long>(i1) * static_cast<unsigned long long>(i2) * static_cast<unsigned long long>(i3); | ||
| std::cout << "Fcoef::create: possible integer overflow in product i1*i2*i3! i1 is " << i1 << ", i2 is " << i2 << ", i3 is " << i3 << ", product is " << product << std::endl; | ||
| } | ||
| if (!(i1 > 0)){ | ||
| std::cout << "i1 must be positive! i1 is " << i1 << std::endl; | ||
| std::cout << "Fcoef::create: i1 must be positive! i1 is " << i1 << std::endl; | ||
| } | ||
| if (!(i2 > 0)){ | ||
| std::cout << "i2 must be positive! i2 is " << i2 << std::endl; | ||
| std::cout << "Fcoef::create: i2 must be positive! i2 is " << i2 << std::endl; | ||
| } | ||
| if (!(i3 > 0)){ | ||
| std::cout << "i3 must be positive! i3 is " << i3 << std::endl; | ||
| std::cout << "Fcoef::create: i3 must be positive! i3 is " << i3 << std::endl; |
There was a problem hiding this comment.
These are error diagnostics; using std::cerr (instead of std::cout) is generally more appropriate and makes it easier to separate normal output from error output in logs/pipes. Consider switching these streams (and keeping the prefixing you added).
|
Nice try! Thanks for your contribution! |
Reminder
Linked Issue
Fix #...
Unit Tests and/or Case Tests for my changes
What's changed?
Any changes of core modules? (ignore if not applicable)