Fix template bugs that prevent rolling gtest DEPS.

Fix template bugs that prevent rolling gtest DEPS.

BUG=none

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

[awong, 2014-07-30 22:22:55]

Do you mind pasting the compile error somewhere? I'm curious what the break is.

[Nick Bray (chromium), 2014-07-30 22:45:05]

On 2014/07/30 22:22:55, awong wrote:
> Do you mind pasting the compile error somewhere? I'm curious what the break
is.

I didn't preserve the mojo error, but there were two in a scratch pad...

obj/base/base_unittests.bind_unittest.o:../../base/bind_unittest.cc:function
base::(anonymous namespace)::BindTest_SupportsAddRefAndRelease_Test::TestBody():
error: undefined reference to
'base::internal::SupportsAddRefAndRelease<base::(anonymous
namespace)::HasRef>::value'


obj/base/base_unittests.bind_unittest.o:../../base/bind_unittest.cc:function
base::(anonymous namespace)::BindTest_SupportsAddRefAndRelease_Test::TestBody():
error: undefined reference to
'base::internal::SupportsAddRefAndRelease<testing::StrictMock<base::(anonymous
namespace)::HasRef> >::value'


obj/base/base_unittests.bind_unittest.o:../../base/bind_unittest.cc:function
base::(anonymous namespace)::BindTest_SupportsAddRefAndRelease_Test::TestBody():
error: undefined reference to
'base::internal::SupportsAddRefAndRelease<base::(anonymous
namespace)::HasRefPrivateDtor>::value'
clang: error: linker command failed with exit code 1 (use -v to see invocation)



/mnt/scratch0/b_used/build/slave/android_clang_dbg/build/src/build/android/arm-linux-androideabi-gold/arm-linux-androideabi-ld:
obj/base/base_unittests.bind_unittest.o: in function base::(anonymous
namespace)::BindTest_SupportsAddRefAndRelease_Test::TestBody():../../base/bind_unittest.cc(.text._ZN4base12_GLOBAL__N_138BindTest_SupportsAddRefAndRelease_Test8TestBodyEv+0x1d8):
error: undefined reference to
'base::internal::SupportsAddRefAndRelease<base::(anonymous
namespace)::HasRef>::value'


/mnt/scratch0/b_used/build/slave/android_clang_dbg/build/src/build/android/arm-linux-androideabi-gold/arm-linux-androideabi-ld:
obj/base/base_unittests.bind_unittest.o: in function base::(anonymous
namespace)::BindTest_SupportsAddRefAndRelease_Test::TestBody():../../base/bind_unittest.cc(.text._ZN4base12_GLOBAL__N_138BindTest_SupportsAddRefAndRelease_Test8TestBodyEv+0x1f4):
error: undefined reference to
'base::internal::SupportsAddRefAndRelease<testing::StrictMock<base::(anonymous
namespace)::HasRef> >::value'


/mnt/scratch0/b_used/build/slave/android_clang_dbg/build/src/build/android/arm-linux-androideabi-gold/arm-linux-androideabi-ld:
obj/base/base_unittests.bind_unittest.o: in function base::(anonymous
namespace)::BindTest_SupportsAddRefAndRelease_Test::TestBody():../../base/bind_unittest.cc(.text._ZN4base12_GLOBAL__N_138BindTest_SupportsAddRefAndRelease_Test8TestBodyEv+0x204):
error: undefined reference to
'base::internal::SupportsAddRefAndRelease<base::(anonymous
namespace)::HasRefPrivateDtor>::value'

[Nick Bray (chromium), 2014-07-30 22:46:46]

Oh, sorry, those were the same error from different builds.  Each of the fixed
places were causing a build failure, however.

Here's a sister change:
https://codereview.chromium.org/429793003/

[viettrungluu, 2014-07-30 23:02:19]

mojo lgtm

On 2014/07/30 22:22:55, awong wrote:
> Do you mind pasting the compile error somewhere? I'm curious what the break
is.

I assume that C++ requires out-of-class declarations of storage for static
member variables, even when templatized.

(This kind of makes sense, sort of. E.g., if you explicitly instantiated a
template, you'd probably also declare storage in exactly one compilation unit.)

[Nick Bray (chromium), 2014-07-30 23:09:46]

On 2014/07/30 23:02:19, viettrungluu wrote:
> mojo lgtm
> 
> On 2014/07/30 22:22:55, awong wrote:
> > Do you mind pasting the compile error somewhere? I'm curious what the break
> is.
> 
> I assume that C++ requires out-of-class declarations of storage for static
> member variables, even when templatized.
> 
> (This kind of makes sense, sort of. E.g., if you explicitly instantiated a
> template, you'd probably also declare storage in exactly one compilation
unit.)

I posted this link in the sister CL:
http://en.wikipedia.org/wiki/One_Definition_Rule
It's possible to get away not declaring the storage in some cases, but as soon
as the member variable is read by a "real" expression, the storage must be
declared.  C++11 changes this rule, but I haven't looked into how.

[awong, 2014-07-30 23:11:18]

Hmm...none of the C++11 typetraits (which is what these are patterened
after) declare storage.

Can you try using the enum pattern?

Example:

  static const bool value = sizeof(Check<Base>(0)) == sizeof(Yes);

becomes

  enum { value  = sizeof(Check<Base>(0)) == sizeof(Yes) };

I would really like to avoid having any storage allocated for these things.

-Albert




On Wed, Jul 30, 2014 at 4:02 PM, <viettrungluu@chromium.org> wrote:

> mojo lgtm
>
>
> On 2014/07/30 22:22:55, awong wrote:
>
>> Do you mind pasting the compile error somewhere? I'm curious what the
>> break
>>
> is.
>
> I assume that C++ requires out-of-class declarations of storage for static
> member variables, even when templatized.
>
> (This kind of makes sense, sort of. E.g., if you explicitly instantiated a
> template, you'd probably also declare storage in exactly one compilation
> unit.)
>
> https://codereview.chromium.org/431533003/
>

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

[Nick Bray (chromium), 2014-07-30 23:45:47]

On 2014/07/30 23:11:18, awong wrote:
> Hmm...none of the C++11 typetraits (which is what these are patterened
> after) declare storage.
> 
> Can you try using the enum pattern?
> 
> Example:
> 
>   static const bool value = sizeof(Check<Base>(0)) == sizeof(Yes);
> 
> becomes
> 
>   enum { value  = sizeof(Check<Base>(0)) == sizeof(Yes) };
> 
> I would really like to avoid having any storage allocated for these things.
> 
> -Albert

I'll cook up the patch to convert it to an enum, but I am not entirely
convinced.

Currently we use this pattern in a number of places, and we're somewhat
inconsistent about declaring storage.
https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&q=%...

The enum pattern is basically unused.
https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&q=%...

C++11 relaxed the rules to only require a storage declaration if an address of
the constant is taken.  Once we go C++11, we can drop the storage decls.

As a practical matter, decent LTO should cull the storage?  And even if it
doesn't we're talking on the order of 1k memory for all of Chrome for all uses
of this pattern?  3-12 bytes for this specific change?

There's also the subtle difference between a const bool data member and an int
enum, but I can't predict if that matters with my current knowledge.  Function
overloading, templates, etc, can theoretically behave differently.

I won't argue, but I am not entirely convinced.

[awong, 2014-07-31 00:07:12]

I took a look at libstdc++'s implement of tr1/type_traits, type_traits, and
then libc++'s implementation of type_traits. They use static constexpr,
static const though libc++ uses the enum pattern for some internal "value"
definitions. That gives me reasonable confidence that all 3 methods are
safe enough. In these two specific files, the type traits classes are
internal and we know how they're being used so the chance of having a
construct that relies on the specific type of value is pretty minimal.

I had avoided the enum version when I first wrote bind largely cause it
didn't read as nice, but I'm unaware of any pragmatic downsides to it. My
preference is to just stick with the enum variant if it compiles since then
we don't need to rely on LTO or anything.

It's still worries me we're running into these compile errors now. These
traits are part of base::internal and I don't think we ever use them
outside of an integral constant-expression. I wonder what changed...

-Albert


On Wed, Jul 30, 2014 at 4:45 PM, <ncbray@chromium.org> wrote:

> On 2014/07/30 23:11:18, awong wrote:
>
>> Hmm...none of the C++11 typetraits (which is what these are patterened
>> after) declare storage.
>>
>
>  Can you try using the enum pattern?
>>
>
>  Example:
>>
>
>     static const bool value = sizeof(Check<Base>(0)) == sizeof(Yes);
>>
>
>  becomes
>>
>
>     enum { value  = sizeof(Check<Base>(0)) == sizeof(Yes) };
>>
>
>  I would really like to avoid having any storage allocated for these
>> things.
>>
>
>  -Albert
>>
>
> I'll cook up the patch to convert it to an enum, but I am not entirely
> convinced.
>
> Currently we use this pattern in a number of places, and we're somewhat
> inconsistent about declaring storage.
> https://code.google.com/p/chromium/codesearch#search/&
> sq=package:chromium&q=%22static%20const%20bool%20value%22&type=cs
>
> The enum pattern is basically unused.
> https://code.google.com/p/chromium/codesearch#search/&
> sq=package:chromium&q=%5Cbenum%5Cs*%7B%5Cs*value%5Cs*=&type=cs
>
> C++11 relaxed the rules to only require a storage declaration if an
> address of
> the constant is taken.  Once we go C++11, we can drop the storage decls.
>
> As a practical matter, decent LTO should cull the storage?  And even if it
> doesn't we're talking on the order of 1k memory for all of Chrome for all
> uses
> of this pattern?  3-12 bytes for this specific change?
>
> There's also the subtle difference between a const bool data member and an
> int
> enum, but I can't predict if that matters with my current knowledge.
>  Function
> overloading, templates, etc, can theoretically behave differently.
>
> I won't argue, but I am not entirely convinced.
>
>
> https://codereview.chromium.org/431533003/
>

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

[Nick Bray (chromium), 2014-07-31 00:09:34]

On 2014/07/31 00:07:12, awong wrote:
> I took a look at libstdc++'s implement of tr1/type_traits, type_traits, and
> then libc++'s implementation of type_traits. They use static constexpr,
> static const though libc++ uses the enum pattern for some internal "value"
> definitions. That gives me reasonable confidence that all 3 methods are
> safe enough. In these two specific files, the type traits classes are
> internal and we know how they're being used so the chance of having a
> construct that relies on the specific type of value is pretty minimal.
> 
> I had avoided the enum version when I first wrote bind largely cause it
> didn't read as nice, but I'm unaware of any pragmatic downsides to it. My
> preference is to just stick with the enum variant if it compiles since then
> we don't need to rely on LTO or anything.
> 
> It's still worries me we're running into these compile errors now. These
> traits are part of base::internal and I don't think we ever use them
> outside of an integral constant-expression. I wonder what changed...
> 
> -Albert
> 
> 
> On Wed, Jul 30, 2014 at 4:45 PM, <mailto:ncbray@chromium.org> wrote:
> 
> > On 2014/07/30 23:11:18, awong wrote:
> >
> >> Hmm...none of the C++11 typetraits (which is what these are patterened
> >> after) declare storage.
> >>
> >
> >  Can you try using the enum pattern?
> >>
> >
> >  Example:
> >>
> >
> >     static const bool value = sizeof(Check<Base>(0)) == sizeof(Yes);
> >>
> >
> >  becomes
> >>
> >
> >     enum { value  = sizeof(Check<Base>(0)) == sizeof(Yes) };
> >>
> >
> >  I would really like to avoid having any storage allocated for these
> >> things.
> >>
> >
> >  -Albert
> >>
> >
> > I'll cook up the patch to convert it to an enum, but I am not entirely
> > convinced.
> >
> > Currently we use this pattern in a number of places, and we're somewhat
> > inconsistent about declaring storage.
> > https://code.google.com/p/chromium/codesearch#search/&
> > sq=package:chromium&q=%22static%20const%20bool%20value%22&type=cs
> >
> > The enum pattern is basically unused.
> > https://code.google.com/p/chromium/codesearch#search/&
> > sq=package:chromium&q=%5Cbenum%5Cs*%7B%5Cs*value%5Cs*=&type=cs
> >
> > C++11 relaxed the rules to only require a storage declaration if an
> > address of
> > the constant is taken.  Once we go C++11, we can drop the storage decls.
> >
> > As a practical matter, decent LTO should cull the storage?  And even if it
> > doesn't we're talking on the order of 1k memory for all of Chrome for all
> > uses
> > of this pattern?  3-12 bytes for this specific change?
> >
> > There's also the subtle difference between a const bool data member and an
> > int
> > enum, but I can't predict if that matters with my current knowledge.
> >  Function
> > overloading, templates, etc, can theoretically behave differently.
> >
> > I won't argue, but I am not entirely convinced.
> >
> >
> > https://codereview.chromium.org/431533003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

I believe it was the internal unit tests blowing up.

So far the enum change looks good, just need to cross-test it with the DEPS
roll.

[Nick Bray (chromium), 2014-07-31 01:52:42]

Enums work like a charm.  PTAL.

[awong, 2014-07-31 04:44:33]

LGTM

Thanks!

[Nick Bray (chromium), 2014-07-31 18:35:51]

The CQ bit was checked by ncbray@chromium.org

[commit-bot: I haz the power, 2014-07-31 18:39:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ncbray@chromium.org/431533003/20001

[commit-bot: I haz the power, 2014-07-31 22:36:31]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Nick Bray (chromium), 2014-08-01 17:10:12]

The CQ bit was unchecked by ncbray@chromium.org

[Nick Bray (chromium), 2014-08-01 17:10:14]

The CQ bit was checked by ncbray@chromium.org

[commit-bot: I haz the power, 2014-08-01 17:11:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ncbray@chromium.org/431533003/20001

[commit-bot: I haz the power, 2014-08-01 22:44:19]

Change committed as 287111