Add ALIGNAS and ALIGNOF macros to ensure proper alignment of StaticMemorySingletonTraits

Add ALIGNAS and ALIGNOF macros to ensure proper alignment of StaticMemorySingletonTraits

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

[jbates, 2012-01-13 01:36:18]

ptal

[Sigurður Ásgeirsson, 2012-01-13 14:32:18]

http://codereview.chromium.org/9186057/diff/7001/base/compiler_specific.h
File base/compiler_specific.h (right):

http://codereview.chromium.org/9186057/diff/7001/base/compiler_specific.h#new...
base/compiler_specific.h:104: #define ALIGNED(byte_alignment)
__declspec(align(byte_alignment))
MSVS wants __declspec to precede the declarator, whereas it looks like GCC wants
__attribute__ to trail it.
I don't see how this macro could be used in a compiler-independent fashion.
Maybe you can do something with a two-argument function that takes a type and an
alignment, and would generally be used in a typedef.
#define ALIGNED_TYPE(type, alignment) __declspec(align(alignment)) type
#define ALIGNED_TYPE(type, alignment) type __attribute(...)

typedef ALIGNED_TYPE(char[sizeof(T)], ALIGNOF(T)) AlignedBuffer;
AlignedBuffer buffer_;

http://codereview.chromium.org/9186057/diff/7001/base/memory/singleton.h
File base/memory/singleton.h (right):

http://codereview.chromium.org/9186057/diff/7001/base/memory/singleton.h#newc...
base/memory/singleton.h:138: static const size_t kBufferSize = sizeof(Type) +
ALIGNOF(Type) - 1;
This is pretty wasteful, as many existing instances will effectively double in
size. This will increase the binary size, increase cache misses etc.
If there isn't a good way to make the the alignment declaration stick to the
buffer declaration, I'd suggest using a new traits class, and using that for
instances that need non-default alignment. Instead of penalizing everyone, this
will only affect those who need non-default alignment.

[jbates, 2012-01-13 17:29:17]

http://codereview.chromium.org/9186057/diff/7001/base/compiler_specific.h
File base/compiler_specific.h (right):

http://codereview.chromium.org/9186057/diff/7001/base/compiler_specific.h#new...
base/compiler_specific.h:104: #define ALIGNED(byte_alignment)
__declspec(align(byte_alignment))
On 2012/01/13 14:32:18, Ruðrugis wrote:
> MSVS wants __declspec to precede the declarator, whereas it looks like GCC
wants
> __attribute__ to trail it.
> I don't see how this macro could be used in a compiler-independent fashion.
> Maybe you can do something with a two-argument function that takes a type and
an
> alignment, and would generally be used in a typedef.
> #define ALIGNED_TYPE(type, alignment) __declspec(align(alignment)) type
> #define ALIGNED_TYPE(type, alignment) type __attribute(...)
> 
> typedef ALIGNED_TYPE(char[sizeof(T)], ALIGNOF(T)) AlignedBuffer;
> AlignedBuffer buffer_;

At least for struct and class, I think GCC allows either. So this seems to work
for both:
class ALIGNED(16) MyClass {...};

I haven't yet tested typedefs or the int array[4] example in MSVC though.

http://codereview.chromium.org/9186057/diff/7001/base/memory/singleton.h
File base/memory/singleton.h (right):

http://codereview.chromium.org/9186057/diff/7001/base/memory/singleton.h#newc...
base/memory/singleton.h:138: static const size_t kBufferSize = sizeof(Type) +
ALIGNOF(Type) - 1;
On 2012/01/13 14:32:18, Ruðrugis wrote:
> This is pretty wasteful, as many existing instances will effectively double in
> size. 

How so? ALIGNOF will typically be 4 or 8 for most singletons.

> This will increase the binary size, increase cache misses etc.
> If there isn't a good way to make the the alignment declaration stick to the
> buffer declaration,

Sadly ALIGNED(ALIGNOF(x)) only works in GCC. Really lame that MSVC complains
about that.

> I'd suggest using a new traits class, and using that for
> instances that need non-default alignment. Instead of penalizing everyone,
this
> will only affect those who need non-default alignment.

If we end up going that route, I think we should bump the alignment of this
class to 8 or 16. As is, if someone has uint64 types in their static singleton,
it may not be guaranteed to be aligned in the 32-bit build (it would depend on
the linker's default alignment for global data types, which is where we may be
getting lucky). A worse case would be vector types, which require 16 byte
alignment for the fast instructions.

[willchan no longer on Chromium, 2012-02-15 01:49:23]

Redirecting to evan@ since he probably understands these compiler issues better
than I.

[Evan Martin, 2012-02-15 21:54:38]

I asked jyasskin about this and he suggested s/ALIGNED/ALIGNAS/ as the latter is
the way it's done in C++11.

The code in general seems like the type of thing jar@ is a good reviewer for.

[jbates, 2012-02-15 22:24:51]

On 2012/02/15 21:54:38, Evan Martin wrote:
> I asked jyasskin about this and he suggested s/ALIGNED/ALIGNAS/ as the latter
is
> the way it's done in C++11.

Sounds good.

> 
> The code in general seems like the type of thing jar@ is a good reviewer for.

There will be a new CL before final review. Siggi and I discussed some potential
solutions offline to avoid the Singleton::kBufferSize padding, but I haven't had
time to try them out yet. Essentially, we could specialize a template for each
alignment and use that in place of the buffer_ member.

[Jeffrey Yasskin (google), 2012-02-15 22:27:19]

http://codereview.chromium.org/9186057/diff/7001/base/memory/singleton.h
File base/memory/singleton.h (right):

http://codereview.chromium.org/9186057/diff/7001/base/memory/singleton.h#newc...
base/memory/singleton.h:118: // We are protected by a memory barrier.
Unrelated to the current CL, but, really? I see a pretty explicit "NoBarrier" up
there, which can bite you on non-x86 platforms.

http://codereview.chromium.org/9186057/diff/7001/base/memory/singleton.h#newc...
base/memory/singleton.h:138: static const size_t kBufferSize = sizeof(Type) +
ALIGNOF(Type) - 1;
On 2012/01/13 17:29:18, jbates wrote:
> On 2012/01/13 14:32:18, Ruðrugis wrote:
> > This will increase the binary size, increase cache misses etc.
> > If there isn't a good way to make the the alignment declaration stick to the
> > buffer declaration,
> 
> Sadly ALIGNED(ALIGNOF(x)) only works in GCC. Really lame that MSVC complains
> about that.

Does MSVC accept ALIGNED(size_t-template-argument)? Does it allow passing
ALIGNOF(x) as a template argument? If so, you may be able to sneak it in that
way.

http://codereview.chromium.org/9186057/diff/7001/base/memory/singleton_unitte...
File base/memory/singleton_unittest.cc (right):

http://codereview.chromium.org/9186057/diff/7001/base/memory/singleton_unitte...
base/memory/singleton_unittest.cc:289: EXPECT_EQ(intptr_t(0), singleton3_addr &
31);
Isn't a better test to check that singleton1_addr == singleton3_addr?

[jbates, 2012-02-15 22:51:33]

http://codereview.chromium.org/9186057/diff/7001/base/memory/singleton.h
File base/memory/singleton.h (right):

http://codereview.chromium.org/9186057/diff/7001/base/memory/singleton.h#newc...
base/memory/singleton.h:118: // We are protected by a memory barrier.
On 2012/02/15 22:27:19, jyasskin1 wrote:
> Unrelated to the current CL, but, really? I see a pretty explicit "NoBarrier"
up
> there, which can bite you on non-x86 platforms.

Hmm. I just assumed this code was already thread safe, but looking at it again,
it appears that the atomic code here doesn't protect from a race between New()
and Delete(). Perhaps New() and Delete() are always called sequentially on the
same thread. Is that the case?

http://codereview.chromium.org/9186057/diff/7001/base/memory/singleton.h#newc...
base/memory/singleton.h:138: static const size_t kBufferSize = sizeof(Type) +
ALIGNOF(Type) - 1;
On 2012/02/15 22:27:19, jyasskin1 wrote:
> On 2012/01/13 17:29:18, jbates wrote:
> > On 2012/01/13 14:32:18, Ruðrugis wrote:
> > > This will increase the binary size, increase cache misses etc.
> > > If there isn't a good way to make the the alignment declaration stick to
the
> > > buffer declaration,
> > 
> > Sadly ALIGNED(ALIGNOF(x)) only works in GCC. Really lame that MSVC complains
> > about that.
> 
> Does MSVC accept ALIGNED(size_t-template-argument)? Does it allow passing
> ALIGNOF(x) as a template argument? If so, you may be able to sneak it in that
> way.

Yeah, hopefully that will work. I'll give it a try soon.

http://codereview.chromium.org/9186057/diff/7001/base/memory/singleton_unitte...
File base/memory/singleton_unittest.cc (right):

http://codereview.chromium.org/9186057/diff/7001/base/memory/singleton_unitte...
base/memory/singleton_unittest.cc:289: EXPECT_EQ(intptr_t(0), singleton3_addr &
31);
On 2012/02/15 22:27:19, jyasskin1 wrote:
> Isn't a better test to check that singleton1_addr == singleton3_addr?

Yes! In fact, I had a nightmare about how broken this test is and fully plan to
fix it :)

[jbates, 2012-02-21 23:39:04]

ptal

http://codereview.chromium.org/9186057/diff/25003/base/memory/singleton.h
File base/memory/singleton.h (left):

http://codereview.chromium.org/9186057/diff/25003/base/memory/singleton.h#old...
base/memory/singleton.h:119: base::subtle::NoBarrier_Store(&dead_, 1);
dead_ is already set to 1 at the beginning of New().

[willchan no longer on Chromium, 2012-02-21 23:42:57]

Jeffrey, I'm going to defer to you here if you don't mind. I'll rubberstamp
when you're cool with it.

On Tue, Feb 21, 2012 at 3:39 PM, <jbates@chromium.org> wrote:

> ptal
>
>
> http://codereview.chromium.**org/9186057/diff/25003/base/**
>
memory/singleton.h<http://codereview.chromium.org/9186057/diff/25003/base/memory/singleton.h>
> File base/memory/singleton.h (left):
>
> http://codereview.chromium.**org/9186057/diff/25003/base/**
>
memory/singleton.h#oldcode119<http://codereview.chromium.org/9186057/diff/25003/base/memory/singleton.h#oldcode119>
> base/memory/singleton.h:119: base::subtle::NoBarrier_Store(**&dead_, 1);
> dead_ is already set to 1 at the beginning of New().
>
>
http://codereview.chromium.**org/9186057/<http://codereview.chromium.org/9186...
>

[Sigurður Ásgeirsson, 2012-02-22 15:31:32]

lgtm with a nit - though I'm not an owner here.

http://codereview.chromium.org/9186057/diff/25003/base/memory/aligned_memory_...
File base/memory/aligned_memory_unittest.cc (right):

http://codereview.chromium.org/9186057/diff/25003/base/memory/aligned_memory_...
base/memory/aligned_memory_unittest.cc:35: RawAlignedMemory<8, 8> raw8;
nice - do you know if is there a cost to this? I assume the compiler needs to
align the stack/frame pointer to the max alignment used in a function - does
that cause fallout in e.g. stack unwinding for crash reports? 
(I'm only curious, not suggesting any measurement or code changes).

http://codereview.chromium.org/9186057/diff/25003/base/memory/singleton.h
File base/memory/singleton.h (right):

http://codereview.chromium.org/9186057/diff/25003/base/memory/singleton.h#new...
base/memory/singleton.h:111: if (base::subtle::NoBarrier_AtomicExchange(&dead_,
1))
I couldn't find a discussion of this anywhere in our headers, but I assume that
NoBarrier means that writes on this core prior to the atomic op are allowed to
be reordered past here?
In any case the implementation looks correct to me, the acquire/release dance in
the singleton proper should be sufficient for static buffers.

http://codereview.chromium.org/9186057/diff/25003/base/memory/singleton_unitt...
File base/memory/singleton_unittest.cc (right):

http://codereview.chromium.org/9186057/diff/25003/base/memory/singleton_unitt...
base/memory/singleton_unittest.cc:290: EXPECT_EQ(intptr_t(0), singleton1_addr &
31);
this test would IMHO be more readable if you'd pull the declarations, casts and
tests together into groups. Some running commentary would be good for the reader
as well.

[Jeffrey Yasskin (google), 2012-02-22 18:38:42]

LGTM. My suggestions below are all taste, and since I'm not on Chromium, Will's
in charge of whether you have to follow them.

http://codereview.chromium.org/9186057/diff/25003/base/memory/aligned_memory.h
File base/memory/aligned_memory.h (right):

http://codereview.chromium.org/9186057/diff/25003/base/memory/aligned_memory....
base/memory/aligned_memory.h:42: void* operator new(size_t size) { \
I'd make these private too, to try to cause a compiler error if they're ever
called.

http://codereview.chromium.org/9186057/diff/25003/base/memory/aligned_memory....
base/memory/aligned_memory.h:49: // Specialization for all alignments is
required because MSVC does not
Consider including an MSVC version number here, so future maintainers know when
to re-evaluate.

http://codereview.chromium.org/9186057/diff/25003/base/memory/aligned_memory....
base/memory/aligned_memory.h:69: RawAlignedMemory<sizeof(Type),
ALIGNOF(Type)>::data_);
"this->data_" should also work.

http://codereview.chromium.org/9186057/diff/25003/base/memory/singleton.h
File base/memory/singleton.h (right):

http://codereview.chromium.org/9186057/diff/25003/base/memory/singleton.h#new...
base/memory/singleton.h:111: if (base::subtle::NoBarrier_AtomicExchange(&dead_,
1))
On 2012/02/22 15:31:32, Ruðrugis wrote:
> I couldn't find a discussion of this anywhere in our headers, but I assume
that
> NoBarrier means that writes on this core prior to the atomic op are allowed to
> be reordered past here?

Both writes and reads can be reordered in either direction. On x86, only the
compiler will actually do such reordering, but on ARM and PPC (dunno about MIPS)
the processor will do it too.

> In any case the implementation looks correct to me, the acquire/release dance
in
> the singleton proper should be sufficient for static buffers.

I think this AtomicExchange is enough for the guarantee
StaticMemorySingletonTraits wants to provide, and that the ordering in Singleton
is enough for its guarantees.

http://codereview.chromium.org/9186057/diff/25003/base/memory/singleton.h#new...
base/memory/singleton.h:114: return new(buffer_.data()) Type;
Since you removed the ()s after "Type", this won't zero-initialize types if they
don't have constructors. You may not actually have any singleton
constructor-less types, but I'd leave the ()s in for safety.

[jbates, 2012-02-22 19:37:15]

http://codereview.chromium.org/9186057/diff/25003/base/memory/aligned_memory.h
File base/memory/aligned_memory.h (right):

http://codereview.chromium.org/9186057/diff/25003/base/memory/aligned_memory....
base/memory/aligned_memory.h:42: void* operator new(size_t size) { \
On 2012/02/22 18:38:42, Jeffrey Yasskin (google) wrote:
> I'd make these private too, to try to cause a compiler error if they're ever
> called.

Done.

http://codereview.chromium.org/9186057/diff/25003/base/memory/aligned_memory....
base/memory/aligned_memory.h:49: // Specialization for all alignments is
required because MSVC does not
On 2012/02/22 18:38:42, Jeffrey Yasskin (google) wrote:
> Consider including an MSVC version number here, so future maintainers know
when
> to re-evaluate.

Done.

http://codereview.chromium.org/9186057/diff/25003/base/memory/aligned_memory....
base/memory/aligned_memory.h:69: RawAlignedMemory<sizeof(Type),
ALIGNOF(Type)>::data_);
On 2012/02/22 18:38:42, Jeffrey Yasskin (google) wrote:
> "this->data_" should also work.

Done.

http://codereview.chromium.org/9186057/diff/25003/base/memory/aligned_memory_...
File base/memory/aligned_memory_unittest.cc (right):

http://codereview.chromium.org/9186057/diff/25003/base/memory/aligned_memory_...
base/memory/aligned_memory_unittest.cc:35: RawAlignedMemory<8, 8> raw8;
On 2012/02/22 15:31:32, Ruðrugis wrote:
> nice - do you know if is there a cost to this? I assume the compiler needs to
> align the stack/frame pointer to the max alignment used in a function - does
> that cause fallout in e.g. stack unwinding for crash reports? 
> (I'm only curious, not suggesting any measurement or code changes).

I'm not sure. It must already happen to a certain extent for base types of
alignment 1, 2, 4, or 8 bytes (or 16 if we count vector types such as __m128).
Seems like the only cost would be padding in the stack frame.

http://codereview.chromium.org/9186057/diff/25003/base/memory/singleton.h
File base/memory/singleton.h (right):

http://codereview.chromium.org/9186057/diff/25003/base/memory/singleton.h#new...
base/memory/singleton.h:114: return new(buffer_.data()) Type;
On 2012/02/22 18:38:42, Jeffrey Yasskin (google) wrote:
> Since you removed the ()s after "Type", this won't zero-initialize types if
they
> don't have constructors. You may not actually have any singleton
> constructor-less types, but I'd leave the ()s in for safety.

Done.

http://codereview.chromium.org/9186057/diff/25003/base/memory/singleton_unitt...
File base/memory/singleton_unittest.cc (right):

http://codereview.chromium.org/9186057/diff/25003/base/memory/singleton_unitt...
base/memory/singleton_unittest.cc:290: EXPECT_EQ(intptr_t(0), singleton1_addr &
31);
On 2012/02/22 15:31:32, Ruðrugis wrote:
> this test would IMHO be more readable if you'd pull the declarations, casts
and
> tests together into groups. Some running commentary would be good for the
reader
> as well.

Done.

[jbates, 2012-02-22 20:13:15]

There might be one problem. If AlignedMemory is not POD, we may get static
initializers generated when it's static. I see that LazyInstance has that as an
important design constraint, so I'm thinking it's best to ditch the
AlignedMemory convenience class and make RawAlignedMemory POD. How does that
sound?

[Sigurður Ásgeirsson, 2012-02-22 20:21:00]

On Wed, Feb 22, 2012 at 15:13, <jbates@chromium.org> wrote:

> There might be one problem. If AlignedMemory is not POD, we may get static
> initializers generated when it's static. I see that LazyInstance has that
> as an
> important design constraint, so I'm thinking it's best to ditch the
> AlignedMemory convenience class and make RawAlignedMemory POD. How does
> that
> sound?
>
>
A POD sounds like a good idea for backing the singletons at least.



>
http://codereview.chromium.**org/9186057/<http://codereview.chromium.org/9186...
>

[Jeffrey Yasskin (google), 2012-02-22 22:04:47]

On 2012/02/22 20:13:15, jbates wrote:
> There might be one problem. If AlignedMemory is not POD, we may get static
> initializers generated when it's static. I see that LazyInstance has that as
an
> important design constraint, so I'm thinking it's best to ditch the
> AlignedMemory convenience class and make RawAlignedMemory POD. How does that
> sound?

As long as AlignedMemory has a trivial default constructor and a trivial
destructor (meaning you didn't declare them, or you define them with =default in
C++11), the compiler shouldn't generate a static initializer, but I haven't
checked that.

[jbates, 2012-02-22 22:12:22]

On 2012/02/22 22:04:47, Jeffrey Yasskin (google) wrote:
> On 2012/02/22 20:13:15, jbates wrote:
> > There might be one problem. If AlignedMemory is not POD, we may get static
> > initializers generated when it's static. I see that LazyInstance has that as
> an
> > important design constraint, so I'm thinking it's best to ditch the
> > AlignedMemory convenience class and make RawAlignedMemory POD. How does that
> > sound?
> 
> As long as AlignedMemory has a trivial default constructor and a trivial
> destructor (meaning you didn't declare them, or you define them with =default
in
> C++11), the compiler shouldn't generate a static initializer, but I haven't
> checked that.

It could be a MSVC quirk then. This is from lazy_instance.h:

  // Do not define a destructor, as doing so makes LazyInstance a
  // non-POD-struct. We don't want that because then a static initializer will
  // be created to register the (empty) destructor with atexit() under MSVC, for
  // example.

Assuming that's true, it's not too bad to just stick with POD. We mainly lose
the elegance of privatizing operator new.

[jbates, 2012-02-22 22:33:16]

On 2012/02/22 22:12:22, jbates wrote:
> On 2012/02/22 22:04:47, Jeffrey Yasskin (google) wrote:
> > On 2012/02/22 20:13:15, jbates wrote:
> > > There might be one problem. If AlignedMemory is not POD, we may get static
> > > initializers generated when it's static. I see that LazyInstance has that
as
> > an
> > > important design constraint, so I'm thinking it's best to ditch the
> > > AlignedMemory convenience class and make RawAlignedMemory POD. How does
that
> > > sound?
> > 
> > As long as AlignedMemory has a trivial default constructor and a trivial
> > destructor (meaning you didn't declare them, or you define them with
=default
> in
> > C++11), the compiler shouldn't generate a static initializer, but I haven't
> > checked that.
> 
> It could be a MSVC quirk then. This is from lazy_instance.h:
> 
>   // Do not define a destructor, as doing so makes LazyInstance a
>   // non-POD-struct. We don't want that because then a static initializer will
>   // be created to register the (empty) destructor with atexit() under MSVC,
for
>   // example.
> 
> Assuming that's true, it's not too bad to just stick with POD. We mainly lose
> the elegance of privatizing operator new.

My mistake, POD can have private methods so I can still make operator new
private.

[Jeffrey Yasskin (google), 2012-02-22 23:10:22]

On Wed, Feb 22, 2012 at 2:12 PM,  <jbates@chromium.org> wrote:
> On 2012/02/22 22:04:47, Jeffrey Yasskin (google) wrote:
>>
>> On 2012/02/22 20:13:15, jbates wrote:
>> > There might be one problem. If AlignedMemory is not POD, we may get
>> > static
>> > initializers generated when it's static. I see that LazyInstance has
>> > that as
>> an
>> > important design constraint, so I'm thinking it's best to ditch the
>> > AlignedMemory convenience class and make RawAlignedMemory POD. How does
>> > that
>> > sound?
>
>
>> As long as AlignedMemory has a trivial default constructor and a trivial
>> destructor (meaning you didn't declare them, or you define them with
>> =default
>
> in
>>
>> C++11), the compiler shouldn't generate a static initializer, but I
>> haven't
>> checked that.
>
>
> It could be a MSVC quirk then. This is from lazy_instance.h:
>
>  // Do not define a destructor, as doing so makes LazyInstance a

I don't know what MSVC actually does for non-PODs, but the important
thing in that comment is the non-trivial (albeit empty) destructor.

>  // non-POD-struct. We don't want that because then a static initializer
> will
>  // be created to register the (empty) destructor with atexit() under MSVC,
> for
>  // example.
>
> Assuming that's true, it's not too bad to just stick with POD. We mainly
> lose
> the elegance of privatizing operator new.
>
> http://codereview.chromium.org/9186057/

[jbates, 2012-02-23 00:41:30]

ptal - I fixed a couple other instances where char* was casted to template
types.

[willchan no longer on Chromium, 2012-02-23 01:18:41]

http://codereview.chromium.org/9186057/diff/24003/base/memory/aligned_memory_...
File base/memory/aligned_memory_unittest.cc (right):

http://codereview.chromium.org/9186057/diff/24003/base/memory/aligned_memory_...
base/memory/aligned_memory_unittest.cc:23: EXPECT_EQ(ALIGNOF(raw8), 8u);
You've got the expectations reversed. It should be expected, actual. Please
reverse the arguments everywhere here.

http://codereview.chromium.org/9186057/diff/24003/base/memory/singleton_unitt...
File base/memory/singleton_unittest.cc (right):

http://codereview.chromium.org/9186057/diff/24003/base/memory/singleton_unitt...
base/memory/singleton_unittest.cc:285: EXPECT_ALIGNED(align4, 4);
Again it's reversed here.

[jbates, 2012-02-23 02:23:53]

http://codereview.chromium.org/9186057/diff/24003/base/memory/aligned_memory_...
File base/memory/aligned_memory_unittest.cc (right):

http://codereview.chromium.org/9186057/diff/24003/base/memory/aligned_memory_...
base/memory/aligned_memory_unittest.cc:23: EXPECT_EQ(ALIGNOF(raw8), 8u);
On 2012/02/23 01:18:41, willchan wrote:
> You've got the expectations reversed. It should be expected, actual. Please
> reverse the arguments everywhere here.

Done.

http://codereview.chromium.org/9186057/diff/24003/base/memory/singleton_unitt...
File base/memory/singleton_unittest.cc (right):

http://codereview.chromium.org/9186057/diff/24003/base/memory/singleton_unitt...
base/memory/singleton_unittest.cc:285: EXPECT_ALIGNED(align4, 4);
On 2012/02/23 01:18:41, willchan wrote:
> Again it's reversed here.

The macro is defined above to call EXPECT_EQ properly.

[willchan no longer on Chromium, 2012-02-23 03:32:05]

lgtm

[commit-bot: I haz the power, 2012-02-23 17:32:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbates@chromium.org/9186057/32007

[commit-bot: I haz the power, 2012-02-23 17:32:41]

Presubmit check for 9186057-32007 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Found Singleton<T> in the following header files.
Please move them to an appropriate source file so that the template gets
instantiated in a single compilation unit.
  base/memory/singleton.h

Presubmit checks took 1.9s to calculate.

[willchan no longer on Chromium, 2012-02-23 17:39:50]

I think you may need to bypass this presubmit.

On Thu, Feb 23, 2012 at 9:32 AM, <commit-bot@chromium.org> wrote:

> Presubmit check for 9186057-32007 failed and returned exit status 1.
>
> Running presubmit commit checks ...
>
> ** Presubmit Messages **
> If this change requires manual test instructions to QA team, add
> TEST=[instructions].
>
> ** Presubmit ERRORS **
> Found Singleton<T> in the following header files.
> Please move them to an appropriate source file so that the template gets
> instantiated in a single compilation unit.
>  base/memory/singleton.h
>
> Presubmit checks took 1.9s to calculate.
>
>
>
>
http://codereview.chromium.**org/9186057/<http://codereview.chromium.org/9186...
>