More switching to standard type traits

more switching to standard type traits

The patch changes removes WTF type trais and changes the client code as following:
WTF::EnableIf -> std::enable_if
WTF::IsSameType -> std::is_same
WTF::IsConvertibleTo.. -> std::is_convertible
WTF::IsPolymorphic -> std::is_polymorphic

BUG=554293
Committed: https://crrev.com/1ccd19c09376043faf412a31b251c319e6f73e2f
Cr-Commit-Position: refs/heads/master@{#367242}

[Mikhail, 2015-12-08 14:03:45]

Patchset #1 (id:1) has been deleted

[Mikhail, 2015-12-08 14:03:55]

Patchset #1 (id:20001) has been deleted

[Mikhail, 2015-12-08 14:04:10]

Patchset #1 (id:40001) has been deleted

[Mikhail, 2015-12-08 14:21:35]

Description was changed from

==========
WIP: more switching to stantard type traits

BUG=554293
==========

to

==========
more switching to standard type traits

The patch changes removes WTF type trais and changes the client code as
following:
WTF::EnableIf -> std::enable_if
WTF::IsSameType -> std::is_same_type
WTF::IsSubclass, WTF::IsConvertibleTo.. -> std::is_convertible


BUG=554293
==========

[Mikhail, 2015-12-08 14:44:31]

Patchset #2 (id:80001) has been deleted

[Mikhail, 2015-12-08 15:02:15]

Description was changed from

==========
more switching to standard type traits

The patch changes removes WTF type trais and changes the client code as
following:
WTF::EnableIf -> std::enable_if
WTF::IsSameType -> std::is_same_type
WTF::IsSubclass, WTF::IsConvertibleTo.. -> std::is_convertible


BUG=554293
==========

to

==========
more switching to standard type traits

The patch changes removes WTF type trais and changes the client code as
following:
WTF::EnableIf -> std::enable_if
WTF::IsSameType -> std::is_same_type
WTF::IsSubclass, WTF::IsConvertibleTo.. -> std::is_convertible
WTF::IsPolymorphic -> std::is_polymorphic


BUG=554293
==========

[Mikhail, 2015-12-08 15:02:38]

Description was changed from

==========
more switching to standard type traits

The patch changes removes WTF type trais and changes the client code as
following:
WTF::EnableIf -> std::enable_if
WTF::IsSameType -> std::is_same_type
WTF::IsSubclass, WTF::IsConvertibleTo.. -> std::is_convertible
WTF::IsPolymorphic -> std::is_polymorphic


BUG=554293
==========

to

==========
more switching to standard type traits

The patch changes removes WTF type trais and changes the client code as
following:
WTF::EnableIf -> std::enable_if
WTF::IsSameType -> std::is_same
WTF::IsSubclass, WTF::IsConvertibleTo.. -> std::is_convertible
WTF::IsPolymorphic -> std::is_polymorphic


BUG=554293
==========

[Mikhail, 2015-12-08 15:08:45]

Patchset #1 (id:60001) has been deleted

[Mikhail, 2015-12-09 08:43:10]

mikhail.pozdnyakov@intel.com changed reviewers:
+ thakis@chromium.org

[Mikhail, 2015-12-09 08:43:27]

PTAL

[Timothy Loh, 2015-12-09 08:48:50]

On 2015/12/09 08:43:27, Mikhail wrote:
> PTAL

Is there a reason to prefer is_convertible over is_base_of?

[Mikhail, 2015-12-09 12:04:44]

On 2015/12/09 08:48:50, Timothy Loh wrote:
> On 2015/12/09 08:43:27, Mikhail wrote:
> > PTAL
> 
> Is there a reason to prefer is_convertible over is_base_of?

In the updated patch I've used is_base_of in most of places, however when we do
need a conversion is_convertible is more preferable as it does not consider
inaccessible bases.

[Nico, 2015-12-09 15:09:39]

Nice! Just one question:

https://codereview.chromium.org/1477213003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/TraceEvent.h (right):

https://codereview.chromium.org/1477213003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/TraceEvent.h:386: template<typename T> struct
ConvertableToTraceFormatTraits<PassRefPtr<T>> {
"Convertable", sigh.

https://codereview.chromium.org/1477213003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/LeakAnnotations.h (right):

https://codereview.chromium.org/1477213003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/LeakAnnotations.h:104: template<typename V> static
YesType checkHasRegisterAsStaticReferenceMethod(V* p, typename
std::enable_if<std::is_base_of<V, typename
std::remove_pointer<decltype(p->registerAsStaticReference())>::type>::value>::Type*
= 0);
Since you're replacing IsSubclass with is_base_of here, don't you need to switch
the order of the arguments?

Since you didn't do this and things appear to work: Do we have to few tests
here? (Or maybe just too few static_assert()s?)

[Mikhail, 2015-12-10 11:38:15]

https://codereview.chromium.org/1477213003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/LeakAnnotations.h (right):

https://codereview.chromium.org/1477213003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/LeakAnnotations.h:104: template<typename V> static
YesType checkHasRegisterAsStaticReferenceMethod(V* p, typename
std::enable_if<std::is_base_of<V, typename
std::remove_pointer<decltype(p->registerAsStaticReference())>::type>::value>::Type*
= 0);
On 2015/12/09 15:09:39, Nico wrote:
> Since you're replacing IsSubclass with is_base_of here, don't you need to
switch
> the order of the arguments?

I do, thanks for noticing. Fixed in the latest patch.
> 
> Since you didn't do this and things appear to work: Do we have to few tests
> here? (Or maybe just too few static_assert()s?)
I failed to find testing code for it. It's probably worth adding but IMO in a
separate patch.

[sof, 2015-12-10 11:42:20]

On 2015/12/10 11:38:15, Mikhail wrote:
>
https://codereview.chromium.org/1477213003/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/wtf/LeakAnnotations.h (right):
> 
>
https://codereview.chromium.org/1477213003/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/wtf/LeakAnnotations.h:104: template<typename V>
static
> YesType checkHasRegisterAsStaticReferenceMethod(V* p, typename
> std::enable_if<std::is_base_of<V, typename
>
std::remove_pointer<decltype(p->registerAsStaticReference())>::type>::value>::Type*
> = 0);
> On 2015/12/09 15:09:39, Nico wrote:
> > Since you're replacing IsSubclass with is_base_of here, don't you need to
> switch
> > the order of the arguments?
> 
> I do, thanks for noticing. Fixed in the latest patch.
> > 
> > Since you didn't do this and things appear to work: Do we have to few tests
> > here? (Or maybe just too few static_assert()s?)
> I failed to find testing code for it. It's probably worth adding but IMO in a
> separate patch.

If you get it wrong, it will leak profusely with LSan and Oilpan on e.g.,
webkit_unit_tests.

[Nico, 2015-12-10 19:46:38]

The win_compile_dbg bot complains:

e:\b\depot_tools\win_toolchain\vs2013_files\vc\include\type_traits(1348) : error
C2139: 'blink::FileList' : an undefined class is not allowed as an argument to
compiler intrinsic type trait '__is_base_of'


Other than that, lgtm. Thanks!

[Yuta Kitamura, 2015-12-11 03:10:34]

On 2015/12/10 19:46:38, Nico wrote:
> The win_compile_dbg bot complains:
> 
> e:\b\depot_tools\win_toolchain\vs2013_files\vc\include\type_traits(1348) :
error
> C2139: 'blink::FileList' : an undefined class is not allowed as an argument to
> compiler intrinsic type trait '__is_base_of'

We have recently discovered that MSVC requires the definition of
Derived in std::is_base_of<Base, Derived> if Base == Derived against
the C++11 specification.

See PatchSet 3 of the following:
https://codereview.chromium.org/1493913004/

See also the comment #26 for a bug in GCC <4.9.

[Mikhail, 2015-12-17 10:26:14]

On 2015/12/10 19:46:38, Nico (office move Wed Thu Fri) wrote:
> The win_compile_dbg bot complains:
> 
> e:\b\depot_tools\win_toolchain\vs2013_files\vc\include\type_traits(1348) :
error
> C2139: 'blink::FileList' : an undefined class is not allowed as an argument to
> compiler intrinsic type trait '__is_base_of'
> 
> 
> Other than that, lgtm. Thanks!

I used std::is_convertible there (in threading traits) to fix that as
std::is_convertible can works with pointers to undefined classes, otherwise
plenty of similar issues turn up. Besides I've made WTF::IsBaseOf trait work
around generic.

Do the above changes need an extra review round?

[Nico, 2015-12-18 13:54:20]

On 2015/12/11 03:10:34, Yuta Kitamura wrote:
> On 2015/12/10 19:46:38, Nico wrote:
> > The win_compile_dbg bot complains:
> > 
> > e:\b\depot_tools\win_toolchain\vs2013_files\vc\include\type_traits(1348) :
> error
> > C2139: 'blink::FileList' : an undefined class is not allowed as an argument
to
> > compiler intrinsic type trait '__is_base_of'
> 
> We have recently discovered that MSVC requires the definition of
> Derived in std::is_base_of<Base, Derived> if Base == Derived against
> the C++11 specification.
> 
> See PatchSet 3 of the following:
> https://codereview.chromium.org/1493913004/
> 
> See also the comment #26 for a bug in GCC <4.9.

Thank you for this pointer, yutak! Do you think we should mention this bug on
chromium-cpp.appspot.com to save others some time in the future? Some of the
"Notes" sections point out compiler bugs and workarounds (see e.g. the std::move
entry). Also, do you know if this works in MSVC2015 and/or if there's an issue
filed with Microsoft about this?

[Nico, 2015-12-18 14:14:14]

https://codereview.chromium.org/1477213003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/ThreadingTraits.h (right):

https://codereview.chromium.org/1477213003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadingTraits.h:39: ||
std::is_convertible<typename std::remove_const<T>::type*, NodeList*>::value>
struct DefaultThreadingTrait;
Now that I think about it, this is a risky change, right? It's a good thing that
is_base_of requires the types to be complete (unless they're the same).
Consider:

#include <type_traits>
class A; class B;
static_assert(std::is_base_of<A, B>::value, "");

This doesn't build, it complains that A and B aren't complete.

Now consider:
#include <type_traits>
class A; class B;
static_assert(std::is_convertible<B*, A*>::value, "");


This builds, and the static_assert fails. Now we add the definitions:

#include <type_traits>
class A; class B;
class A {}; class B : public A {};
static_assert(std::is_convertible<B*, A*>::value, "");

This builds, and the static_assert doesn't fail! This means is_convertible will
have different results based on if classes are defined or not (which is
unpredictable). This is actually fairly worrying. Maybe we shouldn't be using
std::is_convertible much...

[Mikhail, 2015-12-28 14:25:00]

Description was changed from

==========
more switching to standard type traits

The patch changes removes WTF type trais and changes the client code as
following:
WTF::EnableIf -> std::enable_if
WTF::IsSameType -> std::is_same
WTF::IsSubclass, WTF::IsConvertibleTo.. -> std::is_convertible
WTF::IsPolymorphic -> std::is_polymorphic


BUG=554293
==========

to

==========
more switching to standard type traits

The patch changes removes WTF type trais and changes the client code as
following:
WTF::EnableIf -> std::enable_if
WTF::IsSameType -> std::is_same
WTF::IsConvertibleTo.. -> std::is_convertible
WTF::IsPolymorphic -> std::is_polymorphic


BUG=554293
==========

[Mikhail, 2015-12-28 15:16:58]

On 2015/12/18 14:14:14, Nico wrote:
> 
> This builds, and the static_assert doesn't fail! This means is_convertible
will
> have different results based on if classes are defined or not (which is
> unpredictable). This is actually fairly worrying. Maybe we shouldn't be using
> std::is_convertible much...

This might cause problems, however this is how the existing WTF::IsSubclass (and
others) trait are working so far..
std::is_base_of has issues on Win, unfortunately right now I do not have
environment to resolve them, so I would propose to land the rest of the traits
in this patch and handle WTF::IsSubclass/std::is_base_of in a separate CL

[Mikhail, 2015-12-31 12:12:54]

The CQ bit was checked by mikhail.pozdnyakov@intel.com

[Mikhail, 2015-12-31 12:12:55]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1477213003/#ps300001
(title: "Excluded WTF::IsSubclass from the patch")

[commit-bot: I haz the power, 2015-12-31 12:13:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1477213003/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1477213003/300001

[commit-bot: I haz the power, 2015-12-31 13:39:30]

Description was changed from

==========
more switching to standard type traits

The patch changes removes WTF type trais and changes the client code as
following:
WTF::EnableIf -> std::enable_if
WTF::IsSameType -> std::is_same
WTF::IsConvertibleTo.. -> std::is_convertible
WTF::IsPolymorphic -> std::is_polymorphic


BUG=554293
==========

to

==========
more switching to standard type traits

The patch changes removes WTF type trais and changes the client code as
following:
WTF::EnableIf -> std::enable_if
WTF::IsSameType -> std::is_same
WTF::IsConvertibleTo.. -> std::is_convertible
WTF::IsPolymorphic -> std::is_polymorphic


BUG=554293
==========

[commit-bot: I haz the power, 2015-12-31 13:39:31]

Committed patchset #11 (id:300001)

[commit-bot: I haz the power, 2015-12-31 13:40:47]

Description was changed from

==========
more switching to standard type traits

The patch changes removes WTF type trais and changes the client code as
following:
WTF::EnableIf -> std::enable_if
WTF::IsSameType -> std::is_same
WTF::IsConvertibleTo.. -> std::is_convertible
WTF::IsPolymorphic -> std::is_polymorphic


BUG=554293
==========

to

==========
more switching to standard type traits

The patch changes removes WTF type trais and changes the client code as
following:
WTF::EnableIf -> std::enable_if
WTF::IsSameType -> std::is_same
WTF::IsConvertibleTo.. -> std::is_convertible
WTF::IsPolymorphic -> std::is_polymorphic

BUG=554293

Committed: https://crrev.com/1ccd19c09376043faf412a31b251c319e6f73e2f
Cr-Commit-Position: refs/heads/master@{#367242}
==========

[commit-bot: I haz the power, 2015-12-31 13:40:48]

Patchset 11 (id:??) landed as
https://crrev.com/1ccd19c09376043faf412a31b251c319e6f73e2f
Cr-Commit-Position: refs/heads/master@{#367242}