Closed Bug 899319 Opened 6 years ago Closed 6 years ago

consider using the __is_base_of gcc builtin to implement mozilla::IsBaseOf

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Details

Attachments

(1 file, 1 obsolete file)

using gcc's builtin here seems to make things a little faster, my test case was touch mfbt/TypyTraits.h; time make -C dom/bindings/ on the theory that code is fairly template heavy.

unpatched time:
real    23m18.991s
user    22m27.188s
sys     1m9.704s

patched to use is_base_of similar to libstdc++'s
real    23m8.429s
user    22m17.052s
sys     1m9.524s

so it looks like we save on the order of 10 seconds in that directory alone, do we think that's worth the little bit of uglyness?

I checked the type_traits header from libstdc++ 4.4 and it looks like that uses __is_base_of() so I'm going to guess we're safe to use this for all the versions of gcc we care about.
r=me
Sure, why not.

Actually most of the compilers we care about (including MSVC) have intrinsics for this junk, so we could really go to town on this, if we cared, and had time.  I leave it to others to spend their time on this.  ;-)  (Do beware that __is_pod in MSVC has different meaning from __is_pod in other compilers, if you go down that path, tho.)
Attachment #8387364 - Flags: review?(nfroyd)
Comment on attachment 8387364 [details] [diff] [review]
bug 899319 - use __is_base_of() on gcc / clang / msvc

Review of attachment 8387364 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/TypeTraits.h
@@ +488,5 @@
> + */
> +
> +#if defined(__GNUC__) || defined(__clang__) || defined(_MSC_VER)
> +template<class Base, class Derived>
> +struct IsBaseOf : IntegralConstant<bool, __is_base_of(Base, Derived)> {};

This ifdeffery should reside inside the detail namespace, so BaseOfTester is defined in this way for __GNUC__ and friends, and in the current for anything else (if it's even worth trying to support such things...).  Then the comment doesn't have to move and IsBaseOf can just have a single definition.
Attachment #8387364 - Flags: review?(nfroyd) → feedback+
(In reply to Nathan Froyd (:froydnj) from comment #4)
> Comment on attachment 8387364 [details] [diff] [review]
> bug 899319 - use __is_base_of() on gcc / clang / msvc
> 
> Review of attachment 8387364 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mfbt/TypeTraits.h
> @@ +488,5 @@
> > + */
> > +
> > +#if defined(__GNUC__) || defined(__clang__) || defined(_MSC_VER)
> > +template<class Base, class Derived>
> > +struct IsBaseOf : IntegralConstant<bool, __is_base_of(Base, Derived)> {};
> 
> This ifdeffery should reside inside the detail namespace, so BaseOfTester is
> defined in this way for __GNUC__ and friends, and in the current for
> anything else (if it's even worth trying to support such things...).  Then
> the comment doesn't have to move and IsBaseOf can just have a single
> definition.

fwiw I avoided IsBaseOfTester on the theory it would be epsilon faster without another struct in there, but I imagine it hardly mattersso if you prefer the other way that's fine I guess.

As for other things we certainly seem to support the sun compiler enough that I'd think dropping that should be another discussion.
Attachment #8387364 - Attachment is obsolete: true
Attachment #8387778 - Flags: review?(nfroyd)
Comment on attachment 8387778 [details] [diff] [review]
bug 899319 - use __is_base_of() on gcc / clang / msvc

Review of attachment 8387778 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with minor whitespace nits.

::: mfbt/TypeTraits.h
@@ +474,5 @@
>  template<typename T>
>  struct IsSame<T, T> : TrueType {};
>  
>  namespace detail {
> +#if defined(__GNUC__) || defined(__clang__) || defined(_MSC_VER)

MFBT style seems to want whitespace before and after this #if.

@@ +528,5 @@
>  
>  template<class Type>
>  struct BaseOfTester<Type, const Type> : TrueType {};
>  
> +#endif

Whitespace after this too, please.
Attachment #8387778 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/mozilla-central/rev/57da41536eb4
Assignee: nobody → trev.saunders
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.