Make IsBaseOf work on multiply inherited classes as well

RESOLVED FIXED in mozilla19

Status

()

Core
MFBT
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

Trunk
mozilla19
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
Our current IsBaseOf implementation will choke if D derives from multiple B's.  We can use a trick like <http://stackoverflow.com/questions/2910979/how-is-base-of-works> to work around that.

Comment 1

6 years ago
Hmm.  I wonder if, by combining this trick with the existing IsBaseOf trick, we could eliminate the private-inheritance restriction on IsConvertible.  Perhaps a second bug, or second patch.

We should add tests for IsBaseOf while we're doing this, for simple and crazy cases both.
(Assignee)

Comment 2

6 years ago
Created attachment 672527 [details] [diff] [review]
Patch (v1)
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #672527 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 3

6 years ago
Created attachment 672531 [details] [diff] [review]
Patch (v2)

Turns out that the compiler doesn't actually attempt to do the conversion so this even works for private inheritance!
Attachment #672527 - Attachment is obsolete: true
Attachment #672527 - Flags: review?(jwalden+bmo)
Attachment #672531 - Flags: review?(jwalden+bmo)

Comment 4

6 years ago
Comment on attachment 672531 [details] [diff] [review]
Patch (v2)

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

Good stuff.

::: mfbt/TypeTraits.h
@@ +8,4 @@
>  #define mozilla_TypeTraits_h_
>  
>  namespace mozilla {
> +namespace detail {

Blank line before this, please.

@@ +11,5 @@
> +namespace detail {
> +
> +/**
> + * The trickery used to implement IsBaseOf here makes it possible to use it for
> + * the case of multiple inheritence.  This code was inspired by the sample code

"the cases of multiple and private inheritance"

@@ +49,4 @@
>  
>    public:
>      static const bool value =
> +      sizeof(test(mozilla::detail::IsBaseOfHelper<Base, Derived>(), int())) == sizeof(char);

I'd just use detail::IsBaseOfHelper here as you're already in the mozilla namespace.

::: mfbt/tests/TestTypeTraits.cpp
@@ +7,4 @@
>  #include "mozilla/TypeTraits.h"
>  
>  using mozilla::IsConvertible;
> +using mozilla::IsBaseOf;

Let's keep these in alphabetical order.  (Also to coincide with order of use in file, too.)

@@ +21,5 @@
> +{
> +  MOZ_ASSERT((IsBaseOf<A, B>::value),
> +             "A is a base of B");
> +  MOZ_ASSERT((!IsBaseOf<B, A>::value),
> +             "B is not a base of A");

Does IsBaseOf<B, B> work correctly?  Since IsBaseOf is basically trying to be an implementation of C++11 <type_traits> std::is_base_of until we can rely on that header being present, we should include that here if we can.

@@ +34,5 @@
> +  MOZ_ASSERT((!IsBaseOf<A, D>::value),
> +             "A is not a base of D");
> +  MOZ_ASSERT((!IsBaseOf<D, A>::value),
> +             "D is not a base of A");
> +}

Since we're trying to reimplement the C++11 thing in all but name, actually, any chance you could include all the examples from the C++11 spec that we support (and comment out the ones that it happens we don't implement correctly, noting that they're places we're incompatible with <type_traits>)?  For simplicity let's put the examples in their own method called from main(), something like (separate namespace because the spec examples have a struct B and such in them, and TEST THAT THESE WORK because I haven't :-) ):

namespace CPlusPlus11IsBaseOf {

// Adapted from C++11 § 20.9.6.
struct B {};
struct B1 : B {};
struct B2 : B {};
struct D : private B1, private B2 {};

static void
StandardIsBaseOfTests()
{
  MOZ_ASSERT((IsBaseOf<B, D>::value) == true);
  MOZ_ASSERT((IsBaseOf<const B, D>::value) == true);
  MOZ_ASSERT((IsBaseOf<B, const D>::value) == true);
  MOZ_ASSERT((IsBaseOf<B, const B>::value) == true);
  MOZ_ASSERT((IsBaseOf<D, B>::value) == false);
  MOZ_ASSERT((IsBaseOf<B&, D&>::value) == false);
  MOZ_ASSERT((IsBaseOf<B[3], D[3]>::value) == false);
}

}

@@ +76,4 @@
>  main()
>  {
>    TestIsConvertible();
> +  TestIsBaseOf();

Again order these alphabetically/consistent with ordering in the file.
Attachment #672531 - Flags: review?(jwalden+bmo) → review+

Comment 5

6 years ago
Er, } /* namespace CPlusPlus11IsBaseOf */ of course :-)
Is it impossible to use MOZ_STATIC_ASSERT for the assertions?

Comment 7

6 years ago
It could be done, I think.  I don't think it makes a huge amount of difference, except that MOZ_ASSERT will print an error message, while MOZ_STATIC_ASSERT can do so only in certain situations and with certain compilers.  I think I added the original ones here, and I don't remember thinking it through any more than that.
(Assignee)

Comment 8

6 years ago
Created attachment 673097 [details] [diff] [review]
Patch (v3)

This addresses the review comments, and makes all but one of the C++11 tests pass (that one we can also get to pass by specializing IsBaseOf on built-in types -- which might be cumbersome as we need to specialize it on *all* of the combinations of those types, or alternatively implement is_fundamental or something and just && IsFundamental<Base>::value and IsFundamental<Derived>::value in IsBaseOf::value, or something...) perhaps in another bug.
Attachment #672531 - Attachment is obsolete: true
Comment on attachment 673097 [details] [diff] [review]
Patch (v3)

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

Actually, nope, you don't need to specialize on all built-in types -- you just need to attempt to instantiate a class that inherits from Base and Derived.  :-)  (Or maybe two classes, don't remember if common members between the two require virtual inheritance or something funky be used.)  But as I said on IRC last night, we could really go overboard trying to be absolutely entirely completely spec-compliant here, or we could say good enough and defer other tweaks to other bugs, and land this immediate improvement now.  Let's go with this, and we can do any more work we feel like doing in other bugs.

::: mfbt/TypeTraits.h
@@ +12,5 @@
> +namespace detail {
> +
> +/**
> + * The trickery used to implement IsBaseOf here makes it possible to use it for
> + * the cases of multiple inheritence.  This code was inspired by the sample code

This comment still needs fixing.

@@ +72,5 @@
> +  public:
> +    static const bool value = false;
> +};
> +
> +template<class Type>

I think this might allow IsBaseOf<T>, which isn't precisely an issue, but which we might as well not expose if we don't need to.  Will simply template<> do the trick here?

@@ +79,5 @@
> +  public:
> +    static const bool value = true;
> +};
> +
> +template<class Type>

And here.
Attachment #673097 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/9806c99965fb
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
(Assignee)

Comment 12

6 years ago
(Sorry that I landed this patch before seeing this comment.)

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #10)
> ::: mfbt/TypeTraits.h
> @@ +12,5 @@
> > +namespace detail {
> > +
> > +/**
> > + * The trickery used to implement IsBaseOf here makes it possible to use it for
> > + * the cases of multiple inheritence.  This code was inspired by the sample code
> 
> This comment still needs fixing.

What do you want me to fix in there?  It seems to be accurate still.

> @@ +72,5 @@
> > +  public:
> > +    static const bool value = false;
> > +};
> > +
> > +template<class Type>
> 
> I think this might allow IsBaseOf<T>, which isn't precisely an issue, but
> which we might as well not expose if we don't need to.  Will simply
> template<> do the trick here?

No, this is a partial specialization of the template.  It gets selected by the compiler if the Base and Derived types are the same.  The required number of template arguments depends on the argument list after the name of the template class.
(In reply to Ehsan Akhgari [:ehsan] from comment #12)
> > > +/**
> > > + * The trickery used to implement IsBaseOf here makes it possible to use it for
> > > + * the cases of multiple inheritence.  This code was inspired by the sample code
> > 
> > This comment still needs fixing.
> 
> What do you want me to fix in there?  It seems to be accurate still.

"the cases of multiple and private inheritance"

Specifically, mentioning private inheritance, and fixing the "inheritance" misspelling.
(Assignee)

Comment 14

6 years ago
(In reply to comment #13)
> (In reply to Ehsan Akhgari [:ehsan] from comment #12)
> > > > +/**
> > > > + * The trickery used to implement IsBaseOf here makes it possible to use it for
> > > > + * the cases of multiple inheritence.  This code was inspired by the sample code
> > > 
> > > This comment still needs fixing.
> > 
> > What do you want me to fix in there?  It seems to be accurate still.
> 
> "the cases of multiple and private inheritance"
> 
> Specifically, mentioning private inheritance, and fixing the "inheritance"
> misspelling.

Fixed in https://hg.mozilla.org/integration/mozilla-inbound/rev/d04e584c197d.
You need to log in before you can comment on or make changes to this bug.