base: Remove the remaining usage GG_(U)INTn_C macros.

base: Remove the remaining usage of GG_(U)INTn_C macros.

According to comment in base/port.h, in Chromium, we force-define
__STDC_CONSTANT_MACROS, so we can just use the regular (U)INTn_C macros from
<stdint.h>.

BUG=138542
TEST=base_unittests
R=danakj@chromium.org

Committed: https://crrev.com/3208992c8e765e1fe07c1e77f071f3b0a801952f
Cr-Commit-Position: refs/heads/master@{#330440}

[danakj, 2015-05-13 20:25:17]

On Wed, May 13, 2015 at 1:20 PM, <tfarina@chromium.org> wrote:

> Reviewers: danakj,
>
> Description:
> base: Remove the remaining usage GG_(U)INTn_C macros.
>
> According to comment in base/port.h, in Chromium, we force-define
> __STDC_CONSTANT_MACROS, so we can just use the regular (U)INTn_C macros
> from
> <stdint.h>.
>

it's difficult to// guarantee that the INTn_C macros can be defined by
including <stdint.h> at// any specific point.  Provide GG_INTn_C
macros instead.


That doesn't seem to say what I think you're saying. It reads that we have
GG_ macros cuz we don't know that the (U)INT macros will exist?


>
> BUG=138542
> TEST=base_unittests
> R=danakj@chromium.org
>
> Please review this at https://codereview.chromium.org/1128203010/
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+26, -21 lines):
>   M base/containers/hash_tables_unittest.cc
>   M base/rand_util.cc
>   M base/strings/string_number_conversions_unittest.cc
>   M base/time/time_mac.cc
>   M base/time/time_posix.cc
>   M base/time/time_unittest.cc
>   M base/time/time_win.cc
>
>
> Index: base/containers/hash_tables_unittest.cc
> diff --git a/base/containers/hash_tables_unittest.cc
> b/base/containers/hash_tables_unittest.cc
> index
>
60fbeaabc099c7cd967ac5c6c6409160fa0413cf..dd77ac9b1109e2b57985b71d01c984b6f2d055f9
> 100644
> --- a/base/containers/hash_tables_unittest.cc
> +++ b/base/containers/hash_tables_unittest.cc
> @@ -4,6 +4,8 @@
>
>  #include "base/containers/hash_tables.h"
>
> +#include <stdint.h>
> +
>  #include <string>
>
>  #include "base/basictypes.h"
> @@ -31,7 +33,7 @@ TEST_F(HashPairTest, IntegerPairs) {
>    INSERT_PAIR_TEST(Int16Int16Pair, 4, 6);
>    INSERT_PAIR_TEST(Int16Int32Pair, 9, (1 << 29) + 378128932);
>    INSERT_PAIR_TEST(Int16Int64Pair, 10,
> -                   (GG_INT64_C(1) << 60) + GG_INT64_C(78931732321));
> +                   (INT64_C(1) << 60) + INT64_C(78931732321));
>
>    typedef std::pair<int32, int16> Int32Int16Pair;
>    typedef std::pair<int32, int32> Int32Int32Pair;
> @@ -40,7 +42,7 @@ TEST_F(HashPairTest, IntegerPairs) {
>    INSERT_PAIR_TEST(Int32Int16Pair, 4, 6);
>    INSERT_PAIR_TEST(Int32Int32Pair, 9, (1 << 29) + 378128932);
>    INSERT_PAIR_TEST(Int32Int64Pair, 10,
> -                   (GG_INT64_C(1) << 60) + GG_INT64_C(78931732321));
> +                   (INT64_C(1) << 60) + INT64_C(78931732321));
>
>    typedef std::pair<int64, int16> Int64Int16Pair;
>    typedef std::pair<int64, int32> Int64Int32Pair;
> @@ -49,7 +51,7 @@ TEST_F(HashPairTest, IntegerPairs) {
>    INSERT_PAIR_TEST(Int64Int16Pair, 4, 6);
>    INSERT_PAIR_TEST(Int64Int32Pair, 9, (1 << 29) + 378128932);
>    INSERT_PAIR_TEST(Int64Int64Pair, 10,
> -                   (GG_INT64_C(1) << 60) + GG_INT64_C(78931732321));
> +                   (INT64_C(1) << 60) + INT64_C(78931732321));
>  }
>
>  // Verify that base::hash_set<const char*> compares by pointer value, not
> as C
> Index: base/rand_util.cc
> diff --git a/base/rand_util.cc b/base/rand_util.cc
> index
>
1525b91449d438089a67b89f23521941a80e4500..931eb4e943e646e2f39e2848b33683247b5b4725
> 100644
> --- a/base/rand_util.cc
> +++ b/base/rand_util.cc
> @@ -5,6 +5,7 @@
>  #include "base/rand_util.h"
>
>  #include <math.h>
> +#include <stdint.h>
>
>  #include <algorithm>
>  #include <limits>
> @@ -37,7 +38,7 @@ double BitsToOpenEndedUnitInterval(uint64 bits) {
>
>    COMPILE_ASSERT(std::numeric_limits<double>::radix == 2,
> otherwise_use_scalbn);
>    static const int kBits = std::numeric_limits<double>::digits;
> -  uint64 random_bits = bits & ((GG_UINT64_C(1) << kBits) - 1);
> +  uint64 random_bits = bits & ((UINT64_C(1) << kBits) - 1);
>    double result = ldexp(static_cast<double>(random_bits), -1 * kBits);
>    DCHECK_GE(result, 0.0);
>    DCHECK_LT(result, 1.0);
> Index: base/strings/string_number_conversions_unittest.cc
> diff --git a/base/strings/string_number_conversions_unittest.cc
> b/base/strings/string_number_conversions_unittest.cc
> index
>
4787614061ac4145d8b9a448c5714d843b5f8585..0bc72f17ca10d48e0d9945e786c30c05e056b990
> 100644
> --- a/base/strings/string_number_conversions_unittest.cc
> +++ b/base/strings/string_number_conversions_unittest.cc
> @@ -2,6 +2,8 @@
>  // Use of this source code is governed by a BSD-style license that can be
>  // found in the LICENSE file.
>
> +#include "base/strings/string_number_conversions.h"
> +
>  #include <errno.h>
>  #include <stdint.h>
>  #include <stdio.h>
> @@ -10,7 +12,6 @@
>  #include <limits>
>
>  #include "base/format_macros.h"
> -#include "base/strings/string_number_conversions.h"
>  #include "base/strings/stringprintf.h"
>  #include "base/strings/utf_string_conversions.h"
>  #include "testing/gtest/include/gtest/gtest.h"
> @@ -237,10 +238,10 @@ TEST(StringNumberConversionsTest, StringToInt64) {
>      {"42", 42, true},
>      {"-2147483648", INT_MIN, true},
>      {"2147483647", INT_MAX, true},
> -    {"-2147483649", GG_INT64_C(-2147483649), true},
> -    {"-99999999999", GG_INT64_C(-99999999999), true},
> -    {"2147483648", GG_INT64_C(2147483648), true},
> -    {"99999999999", GG_INT64_C(99999999999), true},
> +    {"-2147483649", INT64_C(-2147483649), true},
> +    {"-99999999999", INT64_C(-99999999999), true},
> +    {"2147483648", INT64_C(2147483648), true},
> +    {"99999999999", INT64_C(99999999999), true},
>      {"9223372036854775807", kint64max, true},
>      {"-9223372036854775808", kint64min, true},
>      {"09", 9, true},
> @@ -304,8 +305,8 @@ TEST(StringNumberConversionsTest, StringToUint64) {
>      {"2147483647", INT_MAX, true},
>      {"-2147483649", 0, false},
>      {"-99999999999", 0, false},
> -    {"2147483648", GG_UINT64_C(2147483648), true},
> -    {"99999999999", GG_UINT64_C(99999999999), true},
> +    {"2147483648", UINT64_C(2147483648), true},
> +    {"99999999999", UINT64_C(99999999999), true},
>      {"9223372036854775807", kint64max, true},
>      {"-9223372036854775808", 0, false},
>      {"09", 9, true},
> @@ -327,7 +328,7 @@ TEST(StringNumberConversionsTest, StringToUint64) {
>      {"-", 0, false},
>      {"-9223372036854775809", 0, false},
>      {"-99999999999999999999", 0, false},
> -    {"9223372036854775808", GG_UINT64_C(9223372036854775808), true},
> +    {"9223372036854775808", UINT64_C(9223372036854775808), true},
>      {"99999999999999999999", kuint64max, false},
>      {"18446744073709551615", kuint64max, true},
>      {"18446744073709551616", kuint64max, false},
> @@ -550,7 +551,7 @@ TEST(StringNumberConversionsTest, HexStringToInt64) {
>      {"42", 66, true},
>      {"-42", -66, true},
>      {"+42", 66, true},
> -    {"40acd88557b", GG_INT64_C(4444444448123), true},
> +    {"40acd88557b", INT64_C(4444444448123), true},
>      {"7fffffff", INT_MAX, true},
>      {"-80000000", INT_MIN, true},
>      {"ffffffff", 0xffffffff, true},
> @@ -558,7 +559,7 @@ TEST(StringNumberConversionsTest, HexStringToInt64) {
>      {"0x42", 66, true},
>      {"-0x42", -66, true},
>      {"+0x42", 66, true},
> -    {"0x40acd88557b", GG_INT64_C(4444444448123), true},
> +    {"0x40acd88557b", INT64_C(4444444448123), true},
>      {"0x7fffffff", INT_MAX, true},
>      {"-0x80000000", INT_MIN, true},
>      {"0xffffffff", 0xffffffff, true},
> @@ -607,7 +608,7 @@ TEST(StringNumberConversionsTest, HexStringToUInt64) {
>      {"42", 66, true},
>      {"-42", 0, false},
>      {"+42", 66, true},
> -    {"40acd88557b", GG_INT64_C(4444444448123), true},
> +    {"40acd88557b", INT64_C(4444444448123), true},
>      {"7fffffff", INT_MAX, true},
>      {"-80000000", 0, false},
>      {"ffffffff", 0xffffffff, true},
> @@ -615,14 +616,14 @@ TEST(StringNumberConversionsTest, HexStringToUInt64)
> {
>      {"0x42", 66, true},
>      {"-0x42", 0, false},
>      {"+0x42", 66, true},
> -    {"0x40acd88557b", GG_INT64_C(4444444448123), true},
> +    {"0x40acd88557b", INT64_C(4444444448123), true},
>      {"0x7fffffff", INT_MAX, true},
>      {"-0x80000000", 0, false},
>      {"0xffffffff", 0xffffffff, true},
>      {"0XDeadBeef", 0xdeadbeef, true},
>      {"0x7fffffffffffffff", kint64max, true},
>      {"-0x8000000000000000", 0, false},
> -    {"0x8000000000000000", GG_UINT64_C(0x8000000000000000), true},
> +    {"0x8000000000000000", UINT64_C(0x8000000000000000), true},
>      {"-0x8000000000000001", 0, false},
>      {"0xFFFFFFFFFFFFFFFF", kuint64max, true},
>      {"FFFFFFFFFFFFFFFF", kuint64max, true},
> Index: base/time/time_mac.cc
> diff --git a/base/time/time_mac.cc b/base/time/time_mac.cc
> index
>
e263d07945917f8acf2eb192c9b131c5366e5384..957d513563ce57f4b6627d4069c4ecae8b1da8cb
> 100644
> --- a/base/time/time_mac.cc
> +++ b/base/time/time_mac.cc
> @@ -8,6 +8,7 @@
>  #include <CoreFoundation/CFTimeZone.h>
>  #include <mach/mach.h>
>  #include <mach/mach_time.h>
> +#include <stdint.h>
>  #include <sys/sysctl.h>
>  #include <sys/time.h>
>  #include <sys/types.h>
> @@ -114,7 +115,7 @@ namespace base {
>  //   => Thu Jan 01 00:00:00 UTC 1970
>  //   irb(main):011:0> Time.at(-11644473600).getutc()
>  //   => Mon Jan 01 00:00:00 UTC 1601
> -static const int64 kWindowsEpochDeltaSeconds = GG_INT64_C(11644473600);
> +static const int64 kWindowsEpochDeltaSeconds = INT64_C(11644473600);
>
>  // static
>  const int64 Time::kWindowsEpochDeltaMicroseconds =
> Index: base/time/time_posix.cc
> diff --git a/base/time/time_posix.cc b/base/time/time_posix.cc
> index
>
8b207eb36a9d54ca7f6c3420c42b5755ac100921..3464e8ce9fc9b750d1636ece0b095761465dde9a
> 100644
> --- a/base/time/time_posix.cc
> +++ b/base/time/time_posix.cc
> @@ -137,7 +137,7 @@ struct timespec TimeDelta::ToTimeSpec() const {
>  //   => Thu Jan 01 00:00:00 UTC 1970
>  //   irb(main):011:0> Time.at(-11644473600).getutc()
>  //   => Mon Jan 01 00:00:00 UTC 1601
> -static const int64 kWindowsEpochDeltaSeconds = GG_INT64_C(11644473600);
> +static const int64 kWindowsEpochDeltaSeconds = INT64_C(11644473600);
>
>  // static
>  const int64 Time::kWindowsEpochDeltaMicroseconds =
> Index: base/time/time_unittest.cc
> diff --git a/base/time/time_unittest.cc b/base/time/time_unittest.cc
> index
>
456782c64525c45c4921e3a6c9ecafb457378566..3255ee5be351ee75e897df1755df962b3d2e563f
> 100644
> --- a/base/time/time_unittest.cc
> +++ b/base/time/time_unittest.cc
> @@ -832,7 +832,7 @@ TEST(TimeDelta, WindowsEpoch) {
>    exploded.millisecond = 0;
>    Time t = Time::FromUTCExploded(exploded);
>    // Unix 1970 epoch.
> -  EXPECT_EQ(GG_INT64_C(11644473600000000), t.ToInternalValue());
> +  EXPECT_EQ(INT64_C(11644473600000000), t.ToInternalValue());
>
>    // We can't test 1601 epoch, since the system time functions on Linux
>    // only compute years starting from 1900.
> Index: base/time/time_win.cc
> diff --git a/base/time/time_win.cc b/base/time/time_win.cc
> index
>
d2403f21b7ff5538f0449bde39e423a5777f8998..bfc836627e3e84254b6e200a8103d0c5ad7f029c
> 100644
> --- a/base/time/time_win.cc
> +++ b/base/time/time_win.cc
> @@ -106,7 +106,7 @@ base::LazyInstance<base::Lock>::Leaky g_high_res_lock =
>  // number of leap year days between 1601 and 1970: (1970-1601)/4 excluding
>  // 1700, 1800, and 1900.
>  // static
> -const int64 Time::kTimeTToMicrosecondsOffset =
> GG_INT64_C(11644473600000000);
> +const int64 Time::kTimeTToMicrosecondsOffset = INT64_C(11644473600000000);
>
>  // static
>  Time Time::Now() {
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tfarina, 2015-05-13 20:34:07]

On Wed, May 13, 2015 at 5:24 PM, Dana Jansens <danakj@chromium.org> wrote:

> On Wed, May 13, 2015 at 1:20 PM, <tfarina@chromium.org> wrote:
>
>> Reviewers: danakj,
>>
>> Description:
>> base: Remove the remaining usage GG_(U)INTn_C macros.
>>
>> According to comment in base/port.h, in Chromium, we force-define
>> __STDC_CONSTANT_MACROS, so we can just use the regular (U)INTn_C macros
>> from
>> <stdint.h>.
>>
>
> it's difficult to// guarantee that the INTn_C macros can be defined by
including <stdint.h> at// any specific point.  Provide GG_INTn_C macros
instead.
>
>
> Did you get that from third_party/cld/base/port.h?

So, should we keep the GG_ macros instead and remove viettrungluu's TODO
instead?

-- 
Thiago Farina

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-05-13 20:50:55]

On Wed, May 13, 2015 at 1:34 PM, Thiago Farina <tfarina@chromium.org> wrote:

>
>
> On Wed, May 13, 2015 at 5:24 PM, Dana Jansens <danakj@chromium.org> wrote:
>
>> On Wed, May 13, 2015 at 1:20 PM, <tfarina@chromium.org> wrote:
>>
>>> Reviewers: danakj,
>>>
>>> Description:
>>> base: Remove the remaining usage GG_(U)INTn_C macros.
>>>
>>> According to comment in base/port.h, in Chromium, we force-define
>>> __STDC_CONSTANT_MACROS, so we can just use the regular (U)INTn_C macros
>>> from
>>> <stdint.h>.
>>>
>>
>> it's difficult to// guarantee that the INTn_C macros can be defined by
including <stdint.h> at// any specific point.  Provide GG_INTn_C macros
instead.
>>
>>
>> Did you get that from third_party/cld/base/port.h?
>

Oh I probably did.. I just went to base/port.h in codesearch and didn't
realize there's more than one.


>
> So, should we keep the GG_ macros instead and remove viettrungluu's TODO
> instead?
>
> --
> Thiago Farina
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-05-13 20:51:31]

danakj@chromium.org changed reviewers:
+ viettrungluu@chromium.org

[danakj, 2015-05-13 20:51:31]

+viettrungluu PTAL

[tfarina, 2015-05-14 20:20:00]

Ping?

[viettrungluu, 2015-05-14 21:32:33]

Can you remove the macro definitions themselves from port.h?

https://codereview.chromium.org/1128203010/diff/1/base/containers/hash_tables...
File base/containers/hash_tables_unittest.cc (right):

https://codereview.chromium.org/1128203010/diff/1/base/containers/hash_tables...
base/containers/hash_tables_unittest.cc:11: #include "base/basictypes.h"
You can probably remove this include.

https://codereview.chromium.org/1128203010/diff/1/base/time/time_unittest.cc
File base/time/time_unittest.cc (right):

https://codereview.chromium.org/1128203010/diff/1/base/time/time_unittest.cc#...
base/time/time_unittest.cc:6: 
You should probably include <stdint.h> here too.

https://codereview.chromium.org/1128203010/diff/1/base/time/time_win.cc
File base/time/time_win.cc (right):

https://codereview.chromium.org/1128203010/diff/1/base/time/time_win.cc#newco...
base/time/time_win.cc:39: 
ditto

[tfarina, 2015-05-14 22:07:47]

I cannot remove the macro definitions from port.h yet as there uses outside of
base.

Viettrungluu, Dana raised a concern based on third_party/cld/base/port.h, and
wanted to check with you if it is valid and what we should do about it. See
https://codereview.chromium.org/1128203010/#msg2, could you answer to this
concern that Dana raised?

[viettrungluu, 2015-05-14 22:18:06]

On 2015/05/14 22:07:47, tfarina wrote:
> I cannot remove the macro definitions from port.h yet as there uses outside of
> base.
> 
> Viettrungluu, Dana raised a concern based on third_party/cld/base/port.h, and
> wanted to check with you if it is valid and what we should do about it. See
> https://codereview.chromium.org/1128203010/#msg2, could you answer to this
> concern that Dana raised?

Basically, the problem the old version of port.h (as in cld) refers to is that
one might try to do:

#ifndef __STDC_CONSTANT_MACROS
#define __STDC_CONSTANT_MACROS
#endif
#include <stdint.h>

Now we have those macros, right? ...

Oops, not if someone else included <stdint.h> before us. E.g., if we included
something before <stdint.h>. Or if we're a header file, and someone included
<stdint.h> (transitively) before including us.

We don't have this problem since we just -D__STDC_CONSTANT_MACROS, so all
inclusions of <stdint.h> will provide those macros.

[danakj, 2015-05-14 22:27:52]

Thanks for that explanation. LGTM % viettrungluu

https://codereview.chromium.org/1128203010/diff/1/base/containers/hash_tables...
File base/containers/hash_tables_unittest.cc (right):

https://codereview.chromium.org/1128203010/diff/1/base/containers/hash_tables...
base/containers/hash_tables_unittest.cc:8: 
no whitespace here
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Or...

[tfarina, 2015-05-18 17:09:02]

https://codereview.chromium.org/1128203010/diff/1/base/containers/hash_tables...
File base/containers/hash_tables_unittest.cc (right):

https://codereview.chromium.org/1128203010/diff/1/base/containers/hash_tables...
base/containers/hash_tables_unittest.cc:8: 
On 2015/05/14 22:27:52, danakj wrote:
> no whitespace here
>
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Or...

Done.

https://codereview.chromium.org/1128203010/diff/1/base/containers/hash_tables...
base/containers/hash_tables_unittest.cc:11: #include "base/basictypes.h"
On 2015/05/14 21:32:33, viettrungluu wrote:
> You can probably remove this include.

not yet, as it is still using the int* types from basictypes.h.

https://codereview.chromium.org/1128203010/diff/1/base/time/time_unittest.cc
File base/time/time_unittest.cc (right):

https://codereview.chromium.org/1128203010/diff/1/base/time/time_unittest.cc#...
base/time/time_unittest.cc:6: 
On 2015/05/14 21:32:33, viettrungluu wrote:
> You should probably include <stdint.h> here too.

Done.

https://codereview.chromium.org/1128203010/diff/1/base/time/time_win.cc
File base/time/time_win.cc (right):

https://codereview.chromium.org/1128203010/diff/1/base/time/time_win.cc#newco...
base/time/time_win.cc:39: 
On 2015/05/14 21:32:33, viettrungluu wrote:
> ditto

Done.

[tfarina, 2015-05-18 17:09:31]

The CQ bit was checked by tfarina@chromium.org

[tfarina, 2015-05-18 17:09:32]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1128203010/#ps20001
(title: "review")

[commit-bot: I haz the power, 2015-05-18 17:10:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1128203010/20001

[commit-bot: I haz the power, 2015-05-18 22:14:16]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-05-18 22:44:44]

Patchset 2 (id:??) landed as
https://crrev.com/3208992c8e765e1fe07c1e77f071f3b0a801952f
Cr-Commit-Position: refs/heads/master@{#330440}