Fix Geert’s code review comments.

Except the big one about string IO not being flexible enough.

src/libqof/qof/gnc-numeric.cpp

@@ -44,6 +44,8 @@ extern "C"
 #include "gnc-rational.hpp"
 
 static QofLogModule log_module = "qof";
+
+static const uint8_t max_leg_digits{17};
 static const int64_t pten[] = { 1, 10, 100, 1000, 10000, 100000, 1000000,
                                10000000, 100000000, 1000000000,
                                INT64_C(10000000000), INT64_C(100000000000),
@@ -57,8 +59,8 @@ static const int64_t pten[] = { 1, 10, 100, 1000, 10000, 100000, 1000000,
 int64_t
 powten (unsigned int exp)
 {
-    if (exp > 17)
-        exp = 17;
+    if (exp > max_leg_digits)
+        exp = max_leg_digits;
     return pten[exp];
 }
 
@@ -80,7 +82,8 @@ GncNumeric::GncNumeric(GncRational rr)
 
 GncNumeric::GncNumeric(double d) : m_num(0), m_den(1)
 {
-    if (isnan(d) || fabs(d) > 1e18)
+    static uint64_t max_leg_value{INT64_C(1000000000000000000)};
+    if (isnan(d) || fabs(d) > max_leg_value)
     {
         std::ostringstream msg;
         msg << "Unable to construct a GncNumeric from " << d << ".\n";
@@ -90,9 +93,9 @@ GncNumeric::GncNumeric(double d) : m_num(0), m_den(1)
     auto logval = log10(fabs(d));
     int64_t den;
     if (logval > 0.0)
-        den = powten(18 - static_cast<int>(floor(logval) + 1.0));
+        den = powten((max_leg_digits + 1) - static_cast<int>(floor(logval) + 1.0));
     else
-        den = powten(17);
+        den = powten(max_leg_digits);
     auto num = static_cast<int64_t>(floor(static_cast<double>(den) * d));
 
     if (num == 0)
@@ -297,23 +300,28 @@ GncNumeric::to_string() const noexcept
     return out.str();
 }
 
+bool
+GncNumeric::is_decimal() const noexcept
+{
+    for (unsigned pwr = 0; pwr < max_leg_digits && m_den >= pten[pwr]; ++pwr)
+    {
+        if (m_den == pten[pwr])
+            return true;
+        if (m_den % pten[pwr])
+            return false;
+    }
+    return false;
+}
+
 GncNumeric
 GncNumeric::to_decimal(unsigned int max_places) const
 {
-    if (max_places > 17)
-        max_places = 17;
-    bool is_pwr_ten = true;
-    for (int pwr = 1; pwr <= 17 && m_den > powten(pwr); ++pwr)
-        if (m_den % powten(pwr))
-        {
-            is_pwr_ten = false;
-            break;
-        }
-
-    if (m_num == 0 || (is_pwr_ten && m_den < powten(max_places)))
-        return *this; // Nothing to do.
-    if (is_pwr_ten)
+    if (max_places > max_leg_digits)
+        max_places = max_leg_digits;
+    if (is_decimal())
     {
+        if (m_num == 0 || m_den < powten(max_places))
+            return *this; // Nothing to do.
         /* See if we can reduce m_num to fit in max_places */
         auto excess = m_den / powten(max_places);
         if (m_num % excess)
@@ -1029,7 +1037,8 @@ gnc_numeric_reduce(gnc_numeric in)
 gboolean
 gnc_numeric_to_decimal(gnc_numeric *a, guint8 *max_decimal_places)
 {
-    int max_places =  max_decimal_places == NULL ? 17 : *max_decimal_places;
+    int max_places =  max_decimal_places == NULL ? max_leg_digits :
+        *max_decimal_places;
     try
     {
         GncNumeric an (*a);

src/libqof/qof/gnc-numeric.hpp

@@ -25,6 +25,7 @@
 
 #include <string>
 #include <iostream>
+#include <locale>
 #include "gnc-rational-rounding.hpp"
 
 class GncRational;
@@ -170,8 +171,8 @@ public:
      */
     GncNumeric abs() const noexcept;
     /**
-     * Reduce this to an equivalent fraction with the least common multiple as
-     * the denominator.
+     * Return an equivalent fraction with all common factors between the
+     * numerator and the denominator removed.
      *
      * @return reduced GncNumeric
      */
@@ -226,9 +227,13 @@ public:
      * details.
      */
     std::string to_string() const noexcept;
+    /**
+     * @return true if the denominator is a power of ten, false otherwise.
+     */
+    bool is_decimal() const noexcept;
     /**
      * Convert the numeric to have a power-of-10 denominator if possible without
-     * rounding. Throws a std::rane_error on failure; the message will explain
+     * rounding. Throws a std::range_error on failure; the message will explain
      * the details.
      *
      * @param max_places exponent of the largest permissible denominator.
@@ -250,12 +255,14 @@ public:
     void operator/=(GncNumeric b);
     /* @} */
     /** Compare function
-     *
+     *  \defgroup gnc_numeric_comparison
      *  @param b GncNumeric or int to compare to.
      *  @return -1 if this < b, 0 if ==, 1 if this > b.
+     * @{
      */
     int cmp(GncNumeric b);
     int cmp(int64_t b) { return cmp(GncNumeric(b, 1)); }
+    /** @} */
 private:
     struct round_param
     {
@@ -339,12 +346,21 @@ template <typename charT, typename traits>
 std::basic_ostream<charT, traits>& operator<<(std::basic_ostream<charT, traits>& s, GncNumeric n)
 {
     std::basic_ostringstream<charT, traits> ss;
+    std::locale loc = s.getloc();
+    ss.imbue(loc);
+    char dec_pt = '.';
+    try
+    {
+        dec_pt = std::use_facet<std::numpunct<char>>(loc).decimal_point();
+    }
+    catch(const std::bad_cast& err) {} //Don't do anything, num_sep is already set.
+
     ss.copyfmt(s);
     ss.width(0);
     if (n.denom() == 1)
          ss << n.num();
-    else if (n.denom() % 10 == 0)
-        ss << n.num() / n.denom() << "."
+    else if (n.is_decimal())
+        ss << n.num() / n.denom() << dec_pt
            << (n.num() > 0 ? n.num() : -n.num()) % n.denom();
     else
         ss << n.num() << "/" << n.denom();
@@ -369,7 +385,6 @@ std::basic_ostream<charT, traits>& operator<<(std::basic_ostream<charT, traits>&
 template <typename charT, typename traits>
 std::basic_istream<charT, traits>& operator>>(std::basic_istream<charT, traits>& s, GncNumeric& n)
 {
-
     std::string instr;
     s >> instr;
     if (s)

src/libqof/qof/gnc-rational.hpp

@@ -90,8 +90,8 @@ public:
     /** Make a new GncRational with the opposite sign. */
     GncRational operator-() const noexcept;
     /**
-     * Reduce this to an equivalent fraction with the least common multiple as
-     * the denominator.
+     * Return an equivalent fraction with all common factors between the
+     * numerator and the denominator removed.
      *
      * @return reduced GncRational
      */
@@ -246,7 +246,7 @@ inline GncRational operator+(GncRational a, GncInt128 b)
 }
 inline GncRational operator+(GncInt128 a, GncRational b)
 {
-    return b + GncRational(a, 1);
+    return GncRational(a, 1) + a;
 }
 GncRational operator-(GncRational a, GncRational b);
 inline GncRational operator-(GncRational a, GncInt128 b)
@@ -255,7 +255,7 @@ inline GncRational operator-(GncRational a, GncInt128 b)
 }
 inline GncRational operator-(GncInt128 a, GncRational b)
 {
-    return b - GncRational(a, 1);
+    return GncRational(a, 1) - b;
 }
 GncRational operator*(GncRational a, GncRational b);
 inline GncRational operator*(GncRational a, GncInt128 b)
@@ -264,7 +264,7 @@ inline GncRational operator*(GncRational a, GncInt128 b)
 }
 inline GncRational operator*(GncInt128 a, GncRational b)
 {
-    return b * GncRational(a, 1);
+    return GncRational(a, 1) * b;
 }
 GncRational operator/(GncRational a, GncRational b);
 inline GncRational operator/(GncRational a, GncInt128 b)
@@ -273,7 +273,7 @@ inline GncRational operator/(GncRational a, GncInt128 b)
 }
 inline GncRational operator/(GncInt128 a, GncRational b)
 {
-    return b / GncRational(a, 1);
+    return GncRational(a, 1) / b;
 }
 /** @} */
 #endif //__GNC_RATIONAL_HPP__

src/libqof/qof/test/gtest-gnc-numeric.cpp

@@ -211,6 +211,16 @@ TEST(gncnumeric_stream, output_stream)
     GncNumeric rational_string(123, 456);
     output << rational_string;
     EXPECT_EQ("123/456", output.str());
+    output.imbue(std::locale("de_DE"));
+    output.str("");
+    output << simple_int;
+    EXPECT_EQ("123456", output.str());
+    output.str("");
+    output << decimal_string;
+    EXPECT_EQ("123,456", output.str());
+    output.str("");
+    output << rational_string;
+    EXPECT_EQ("123/456", output.str());
 }
 
 TEST(gncnumeric_stream, input_stream)
@@ -491,6 +501,14 @@ TEST(gncnumeric_functions, test_convert)
     EXPECT_EQ(100, c.denom());
 }
 
+TEST(gnc_numeric_functions, test_is_decimal)
+{
+    EXPECT_TRUE(GncNumeric(123, 1).is_decimal());
+    EXPECT_FALSE(GncNumeric(123, 3).is_decimal());
+    EXPECT_TRUE(GncNumeric(123, 1000).is_decimal());
+    EXPECT_FALSE(GncNumeric(123, 3200).is_decimal());
+}
+
 TEST(gnc_numeric_functions, test_conversion_to_decimal)
 {
     GncNumeric a(123456789, 1000), r;