Atomic ops cleanup.

Atomic ops cleanup.

Use <stdint.h> instead of "base/basictypes.h".
Put AtomicOps_Internalx86CPUFeaturesInit in the anonymous namespace.
Fix formatting issues.

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=249938

[cosmin.truta, 2014-02-06 01:38:25]

This is a follow-up on issue 156233002. I cleaned up some of the surrounding
code as I was working on atomic ops.

About testing:
I run tests on my development machine (Linux x64) to make sure I don't break
anything with the replacement of "basictypes.h" with <stdint.h>, and I only had
to correct one file (base/threading/thread_collision_warner.h). I also submitted
a try job with "git cl try", but all try bots show up in red, not sure why.

[Mark Mentovai, 2014-02-06 03:04:13]

You don’t have try server access.
http://www.chromium.org/developers/testing/try-server-usage

I submitted this to the try servers. I haven’t given much thought to the change
itself yet.

[cosmin.truta, 2014-02-06 03:20:03]

I see. Thank you.

[Mark Mentovai, 2014-02-06 14:12:43]

The Windows trybots don’t seem to like this very much.

[cosmin.truta, 2014-02-06 15:27:04]

On 2014/02/06 14:12:43, Mark Mentovai wrote:
> The Windows trybots don’t seem to like this very much.

Indeed, and I am trying to understand why Atomic64 isn't as wide as PVOID on
_WIN64. The relevant change that I made is the replacement of #include
"basictypes.h" with <stdint.h>, but "basictypes.h" is merely a wrapper to
<stdint.h>. I did change the definition of Atomic32; but not Atomic64, which was
already a typedef to either int64_t or intptr_t (both of which should work).

I will set up a Windows build of Chromium and find out why (e.g. by looking at
the preprocessed headers), because I can't tell just from the inspection of the
bots' output.

[Mark Mentovai, 2014-02-06 16:22:54]

Cosmin Truta wrote:
> Indeed, and I am trying to understand why Atomic64 isn't as wide as PVOID on
> _WIN64. The relevant change that I made is the replacement of #include
> "basictypes.h" with <stdint.h>, but "basictypes.h" is merely a wrapper to
> <stdint.h>. I did change the definition of Atomic32; but not Atomic64, which
was
> already a typedef to either int64_t or intptr_t (both of which should work).
> 
> I will set up a Windows build of Chromium and find out why (e.g. by looking at
> the preprocessed headers), because I can't tell just from the inspection of
the
> bots' output.

It’s because the implicated line is

COMPILE_ASSERT(sizeof(Atomic64) == sizeof(PVOID), atomic_word_is_atomic);

and COMPILE_ASSERT is #defined by (a further #include of) base/basictypes.h.

MSVC’s error is obtuse here, but the best hint about what it thinks is happening
is when it says “missing type specifier - int assumed. Note: C++ does not
support default-int”. That hint lets you know that it thinks it’s looking at a
declaration.

[cosmin.truta, 2014-02-06 19:03:50]

On 2014/02/06 16:22:54, Mark Mentovai wrote:
> It’s because the implicated line is
> COMPILE_ASSERT(sizeof(Atomic64) == sizeof(PVOID), atomic_word_is_atomic);
> and COMPILE_ASSERT is #defined by (a further #include of) base/basictypes.h.

Indeed! The error message wasn't helpful, and I can see that by trying to
compile:

Foo(true, Bar);

It's the same kind of error message, saying nothing about Foo (only about Bar)
being undefined. I might be probably expecting the good ol' K&R C behavior...

This also reminds me how good is the include-what-you-use rule.

Mark, I hope that landing this patch won't take much more of your reviewer time.
This small refactoring wasn't that important, and I could have waited to be a
committer myself and then submit to try bots, etc. I *think* the patch is good
to go now, but I'm equally okay with postponing it also, if there are more
foreseeable issues.

[Mark Mentovai, 2014-02-06 19:46:55]

Let’s feed it to the bots again and see what happens.

[cosmin.truta, 2014-02-07 01:26:10]

+glider, +leng for TSan and Mac.

I kept the C style (Type *arg) in the extern "C" section in TSan, but
I used the C++ style (Type* arg) everywhere else. I wanted to be sure
I am not inadvertently reverting their intentions.

[Alexander Potapenko, 2014-02-07 08:55:23]

On 2014/02/07 01:26:10, cosmin.truta wrote:
> +glider, +leng for TSan and Mac.
> 
> I kept the C style (Type *arg) in the extern "C" section in TSan, but
> I used the C++ style (Type* arg) everywhere else. I wanted to be sure
> I am not inadvertently reverting their intentions.

I'm not sure it's necessary to fix the formatting in atomicops_internals_tsan.h,
but it definitely should be consistent across the whole file.
I'd suggest to run clang-format on this file to avoid discussing style issues.

[Alexander Potapenko, 2014-02-07 08:56:51]

CC=dvyukov

> I'm not sure it's necessary to fix the formatting in
atomicops_internals_tsan.h,
> but it definitely should be consistent across the whole file.
> I'd suggest to run clang-format on this file to avoid discussing style issues.

[Dmitry Vyukov, 2014-02-07 09:03:51]

LGTM for atomicops_internals_tsan.h

[cosmin.truta, 2014-02-07 15:32:36]

On 2014/02/07 08:55:23, Alexander Potapenko wrote:
> I'm not sure it's necessary to fix the formatting in
> atomicops_internals_tsan.h,
> but it definitely should be consistent across the whole file.
> I'd suggest to run clang-format on this file to avoid discussing
> style issues.

Ok, clang-format indeed wants its C++ style, and now I see that the
rest of Chromium code uses this C++ style even in extern "C".

What I had originally intended was to bring some of the formatting
changes they had done in V8 back to Chromium, and to make them more
consistent along the way. Counter-intuitively, coding is actually
easier than styling...

[Dmitry Vyukov, 2014-02-07 15:37:41]

On 2014/02/07 15:32:36, cosmin.truta wrote:
> On 2014/02/07 08:55:23, Alexander Potapenko wrote:
> > I'm not sure it's necessary to fix the formatting in
> > atomicops_internals_tsan.h,
> > but it definitely should be consistent across the whole file.
> > I'd suggest to run clang-format on this file to avoid discussing
> > style issues.
> 
> Ok, clang-format indeed wants its C++ style, and now I see that the
> rest of Chromium code uses this C++ style even in extern "C".
> 
> What I had originally intended was to bring some of the formatting
> changes they had done in V8 back to Chromium, and to make them more
> consistent along the way. Counter-intuitively, coding is actually
> easier than styling...

... unless you program in Go, where the styling problem does not exist ;)

[leng, 2014-02-07 16:32:54]

atomicops_internals_mac.h  LGTM

[Mark Mentovai, 2014-02-07 16:53:17]

LGTM

This is too many reviewers for what amounts to formatting cleanup.

[Mark Mentovai, 2014-02-07 16:53:21]

The CQ bit was checked by mark@chromium.org

[commit-bot: I haz the power, 2014-02-07 16:53:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ctruta@blackberry.com/143273005/540001

[commit-bot: I haz the power, 2014-02-07 22:12:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ctruta@blackberry.com/143273005/540001

[commit-bot: I haz the power, 2014-02-08 04:59:57]

Change committed as 249938