Opened 7 years ago

Closed 7 years ago

#3015 closed defect (fixed)

Change type of IntElement to int64_t

Reported by: fujiwara Owned by: fujiwara
Priority: medium Milestone: Sprint-20130820
Component: ~Inter-module communication(obsolete) Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 1 Add Hours to Ticket: 0
Total Hours: 0.90 Internal?: no

Description

The size of IntElement? depends on machine architecture now.
This ticket proposes to change type of IntElement? to int64_t.

Subtickets

Change History (17)

comment:1 Changed 7 years ago by fujiwara

  • Summary changed from Change IntElement type to int64_t to Change type of IntElement to int64_t

comment:2 Changed 7 years ago by fujiwara

  • Milestone changed from New Tasks to Next-Sprint-Proposed

comment:3 Changed 7 years ago by muks

  • Estimated Difficulty changed from 0 to 3

comment:4 Changed 7 years ago by fujiwara

  • Owner set to UnAssigned
  • Status changed from new to reviewing

comment:5 Changed 7 years ago by fujiwara

Branch trac3015 is ready for reviewing.

comment:6 Changed 7 years ago by shane

  • Milestone changed from Previous-Sprint-Proposed to Next-Sprint-Proposed

comment:7 Changed 7 years ago by muks

  • Estimated Difficulty changed from 3 to 1

Re-estimated for review.

comment:8 Changed 7 years ago by muks

  • Milestone changed from Next-Sprint-Proposed to Sprint-20130806

comment:9 Changed 7 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:10 follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to fujiwara

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).

Element::getValue(int64_t&) const {
    return (false);
}
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.

//
// 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?

    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.

-    // 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):

    EXPECT_NO_THROW(el->intValue());

comment:11 in reply to: ↑ 10 ; follow-up: Changed 7 years ago by fujiwara

Thanks for reviewing,

Replying to vorner:

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).

The reason I chose int64_t is that many of us wanted to use 'int64_t'.
int64_t is 64bit integer.
size of long long is equal to or larger than size of int64_t.

The reason I did not use int64_t in some parts is that C++ overloading and automatic type conversion do not work well for derived types (int64_t or int32_t).
Overloading function definition side, we cannot define these five functions f(int64_t), f(int32_t), f(long long), f(long), f(int) because some types are the same sizes.
Referencing side, f(long long) does not match f(int64_t).

Then I chose that I defined f(long long), f(long) and f(int).
Though sizeof(long) may be equal to long long or int (depends on machine architecture),
we can define three functions because they are base types of C++.
f(int64_t) is linked to f(long long).
f(int32_t) is lonked to f(int).

This comment seems to be for an intermediate version of the code, it is no longer true.

//
// 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.
//

Removed it.

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?

    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))); };

C++ overloading and automatic type conversion do not work well if there are string, double and integer.
On definition side, if we add setValue(int64_t), we cannot add setValue(long long), and referencing side, setValue(long long) does not match setValue(int64_t).

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.

LLONG_MAX is not always equal to 231-1 because sizeof(long long) is not always 64bit.
C++ does not have constant of int64_t maximum value.

Should the test check the return value (seen at two places):

    EXPECT_NO_THROW(el->intValue());

I will add.

comment:12 Changed 7 years ago by fujiwara

  • Owner changed from fujiwara to vorner

comment:13 in reply to: ↑ 11 ; follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to fujiwara
  • Total Hours changed from 0 to 0.90

Hello

Replying to fujiwara:

The reason I did not use int64_t in some parts is that C++ overloading and automatic type conversion do not work well for derived types (int64_t or int32_t).
Overloading function definition side, we cannot define these five functions f(int64_t), f(int32_t), f(long long), f(long), f(int) because some types are the same sizes.

OK, now I see. So it was indeed a decision, not just oversight of inconsistency.

Can you add a short note about that (being it a decision and hint for the reason) somewhere near the function definitions, so someone will not try to „fix“ the inconsistency in future and get himself bitten by compiler?

After that, it can be merged.

comment:14 in reply to: ↑ 13 Changed 7 years ago by fujiwara

Replying to vorner:

Can you add a short note about that (being it a decision and hint for the reason) somewhere near the function definitions, so someone will not try to „fix“ the inconsistency in future and get himself bitten by compiler?

After that, it can be merged.

I added Notes on src/lib/cc/data.h.

comment:15 Changed 7 years ago by fujiwara

  • Owner changed from fujiwara to vorner

comment:16 Changed 7 years ago by vorner

  • Owner changed from vorner to fujiwara

Thanks, please merge.

comment:17 Changed 7 years ago by fujiwara

  • Resolution set to fixed
  • Status changed from reviewing to closed

Thanks, merged.

Note: See TracTickets for help on using tickets.