BIND 10 #3015: Change type of IntElement to int64_t

[BIND 10 Development]

#3015: Change type of IntElement to int64_t
-------------------------------------+-------------------------------------
            Reporter:  fujiwara      |                        Owner:
                Type:  defect        |  fujiwara
            Priority:  medium        |                       Status:
           Component:  Inter-module  |  reviewing
  communication                      |                    Milestone:
            Keywords:                |  Sprint-20130806
           Sensitive:  0             |                   Resolution:
         Sub-Project:  Core          |                 CVSS Scoring:
Estimated Difficulty:  1             |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => fujiwara


Comment:

 Hello

 Why do you use `int64_t` in some method parameters and `long long int` at
 others? That looks inconsistent, and relies on an assumption that might
 but does not have to be true (that `int64_t` and `long long` is the same
 type).

 {{{#!c++
 Element::getValue(int64_t&) const {
     return (false);
 }
 }}}

 {{{#!c++
 bool
 Element::setValue(const long long int) {
     return (false);
 }
 }}}

 This comment seems to be for an intermediate version of the code, it is no
 longer true.
 {{{#!c++
 //
 // Type of IntElement is changed from long int to int64_t.
 // However, strtoint64_t function does not exist.
 // It is assumed that "long long" and int64_t are the same sizes.
 // strtoll is used to convert string to integer.
 //
 }}}

 Is this overloading for all these types necessary? Won't it just auto-
 expand the int type? Again, why is it using `long long` here and not
 `int64_t`?
 {{{#!c++
     bool setValue(const long int i) { return (setValue(static_cast<long
 long int>(i))); };
     bool setValue(const int i) { return (setValue(static_cast<long long
 int>(i))); };
 }}}

 Looking at this change, I wonder if it would be better to use constants
 `LLONG_MAX` and `LLONG_MIN` instead of hardcoding the numbers into the
 test.
 {{{#!diff
 -    // LONG_MAX, -LONG_MAX, LONG_MIN test
 -    std::ostringstream longmax, minus_longmax, longmin;
 -    longmax << LONG_MAX;
 -    minus_longmax << -LONG_MAX;
 -    longmin << LONG_MIN;
 -    EXPECT_NO_THROW( {
 -       EXPECT_EQ(longmax.str(), Element::fromJSON(longmax.str())->str());
 +    EXPECT_NO_THROW({
 +       EXPECT_EQ("9223372036854775807",
 Element::fromJSON("9223372036854775807")->str());
      });
 -    EXPECT_NO_THROW( {
 -       EXPECT_EQ(minus_longmax.str(),
 Element::fromJSON(minus_longmax.str())->str());
 -    });
 -    EXPECT_NO_THROW( {
 -       EXPECT_EQ(longmin.str(), Element::fromJSON(longmin.str())->str());
 +    EXPECT_NO_THROW({
 +       EXPECT_EQ("-9223372036854775808",
 Element::fromJSON("-9223372036854775808")->str());
      });
 +    EXPECT_THROW({
 +       EXPECT_NE("9223372036854775808",
 Element::fromJSON("9223372036854775808")->str());
 +    }, JSONError);

      // number underflow
 -    EXPECT_THROW(Element::fromJSON("1.1e-12345678901234567890")->str(),
 JSONError);
 +    //
 EXPECT_THROW(Element::fromJSON("1.1e-12345678901234567890")->str(),
 JSONError);

  }
 }}}

 Should the test check the return value (seen at two places):
 {{{#!c++
     EXPECT_NO_THROW(el->intValue());
 }}}

-- 
Ticket URL: <http://bind10.isc.org/ticket/3015#comment:10>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development