velnias75 / rational

A C++ rational (fraction) template class
GNU General Public License v3.0
8 stars 0 forks source link

Remarks of Melchoir via Reddit #1

Open velnias75 opened 8 years ago

velnias75 commented 8 years ago

From Reddit:

Very cool!

For documentation: It would be good to describe more explicitly the range of storage types that are expected to work. Can I use polynomials and get rational functions? Can I use Gaussian integers and get Gaussian rationals? Ideally, T would not need to have comparison methods as long as I never reference Rational's comparison methods, but maybe they're needed internally for other purposes.

If the above are meant to be supported, it would be good to see a couple of test cases, too. Hopefully, the exotic storage types wouldn't have to be fully fleshed out if they're just for testing.

Also for test cases: Make sure you've handled the most negative value M of a signed binary integer type, since -M = M, which might cause trouble in lots of places.

Back to docs, it would be good to describe what it means to support GMP. What does one gain by using a Rational of GMP integers, rather than a native GMP rational?

On the implementation side, I don't think you need to declare your own [copy, move] [constructor, assignment operator]s or destructor. It looks like your implementations aren't doing anything special.

velnias75 commented 8 years ago

removed copy/move constructor and assignment operator in commit https://github.com/velnias75/rational/commit/316240414cbdf42e22b3e6ec1d135d90b229e368

Destructor is left, because of inlining issues with GMP.