Bind() doesn't work with protected and private inheritance

[wxtrac]

Issue migrated from trac ticket # 17623

component: base | priority: low | resolution: fixed | keywords: work-needed

2016-08-10 11:28:32: tambre (Raul Tambre) created the issue

---

Whenever a class is derived from a class for which events can be bound and the inheritance of the class is either private or protected, the Bind() fails with a compile-time error about the conversion to wxEvtHandler* being inaccessible.

The error message on MSVC14 is as follows:

wxWidgets\include\wx/meta/convertible.h(31): error C2243: 'type cast': conversion from 'Test *' to 'wxEvtHandler *' exists, but is inaccessible
  wxWidgets\include\wx/event.h(335): note: see reference to class template instantiation 'wxConvertibleTo<Class,wxEvtHandler>' being compiled
          with
          [
              Class=Test
          ]
  wxWidgets\include\wx/event.h(3568): note: see reference to class template instantiation 'wxEventFunctorMethod<EventTag,Test,EventArg,EventHandler>' being compiled
          with
          [
              EventTag=wxEventTypeTag<wxSizeEvent>,
              EventArg=wxSizeEvent,
              EventHandler=Test
          ]
  Test.cpp(78): note: see reference to function template instantiation 'void wxEvtHandler::Bind<wxEventTypeTag<wxSizeEvent>,Test,wxSizeEvent,Test>(const EventTag &,void (__cdecl Test::* )(EventArg &),EventHandler *,int,int,wxObject *)' being compiled
          with
          [
              EventTag=wxEventTypeTag<wxSizeEvent>,
              EventArg=wxSizeEvent,
              EventHandler=Test
          ]

The issue was discovered as a bug in this SO question: http://stackoverflow.com/questions/38833116/conversion-in-derived-class-inaccessible-if-base-class-is-protected

The error is caused by the wxConvertibleTo template incorrectly not detecting that protected and private inheritance is also fine.

I have attached a minimal sample and a patch for C++11 versions. I would like to get the patch merged, but I'm guessing it would likely require making it work with the ancient compilers wxWidgets supports.

[wxtrac]

2016-08-10 11:42:29: @vadz changed status from new to confirmed

2016-08-10 11:42:29: @vadz commented

It's not even about the ancient compilers, a relatively recent g++4 won't build this code without -std=c++11 option. So while we could use this code with the checks for C++11, it seems it would be better to have a SFINAE-based solution that would work in non-C++11 build too.

BTW, why are the casts additions/changes needed?

P.S. For other people stumbling upon this problem, there is a simple workaround in my answer to the linked SO question.

[wxtrac]

2016-08-10 12:01:14: tambre (Raul Tambre) commented

---

The first and second cast are needed because inheritance is protected/private so it can't be directly cast, but reinterpret_cast can be used, since it doesn't care.
Note that both of these will never be reached if the derivation check fails, so using reinterpret_cast should be fine.

I unfortunately lack knowledge to implement a SFINAE-based solution, so a SFINAE-based fix will probably be up to someone else.

[wxtrac]

2016-08-12 17:50:15: tambre (Raul Tambre) uploaded file wxWidgets Bind Sample.cpp (0.7 KiB)

A sample to reproduce the problem.

[wxtrac]

2016-08-12 17:55:43: tambre (Raul Tambre) commented

---

I've attached a new SFINAE based solution that also should work on the old compilers.

[wxtrac]

2016-08-17 17:27:39: @vadz commented

---

Hmm, I'm still getting

error: 'wxEvtHandler' is an inaccessible base of 'Test'

with this patch for my simple test case:

#!cpp
#include <wx/frame.h>

class Test : protected wxFrame {
public:
    Test() { Bind(wxEVT_SIZING, &Test::sizing, this); }

private:
    void sizing(wxSizeEvent&);
};

with g++ 4.7.2. I'll try to look into this in more details later...

P.S. Could you please use UTF-8 (preferably without BOM) encoding for the patch files instead of UTF-16 that Git doesn't grok? TIA!

[wxtrac]

2016-08-18 13:26:11: tambre (Raul Tambre) commented

---

Weird, it works just fine for me on MSVC14 with the patch applied. Could you try applying the patch again?

I regenerated the patch and git seemed to output it in UCS2-LE BOM according to Notepad++. I've converted the patch to UTF-8 (without BOM) and have reuploaded it.

[wxtrac]

2016-08-19 23:11:08: @vadz commented

---

Sorry, still doesn't work with g++ (any version I tried, from 4, 5 and 6, but this particular output is from 6):

In file included from include/wx/event.h:32:0,
                 from include/wx/window.h:18,
                 from include/wx/nonownedwnd.h:14,
                 from include/wx/toplevel.h:20,
                 from include/wx/frame.h:18,
                 from protbind.cpp:1:
include/wx/meta/convertible.h:34:12: error: declaration of template parameter 'B' shadows template parameter
  template <typename B, typename D>
            ^~~~~~~~
include/wx/meta/convertible.h:31:11: note: template parameter 'B' declared here
 template <typename B, typename D>
           ^~~~~~~~
include/wx/meta/convertible.h:34:24: error: declaration of template parameter 'D' shadows template parameter
  template <typename B, typename D>
                        ^~~~~~~~
include/wx/meta/convertible.h:31:23: note: template parameter 'D' declared here
 template <typename B, typename D>
                       ^~~~~~~~
In file included from include/wx/window.h:18:0,
                 from include/wx/nonownedwnd.h:14,
                 from include/wx/toplevel.h:20,
                 from include/wx/frame.h:18,
                 from protbind.cpp:1:
include/wx/event.h: In instantiation of 'static T* wxPrivate::HandlerImpl<T, A, true>::ConvertFromEvtHandler(wxEvtHandler*) [with T # Test; AwxSizeEvent]':
include/wx/event.h:364:25:   required from 'void wxEventFunctorMethod<EventTag, Class, EventArg, EventHandler>::operator()(wxEvtHandler*, wxEvent&) [with EventTag # wxEventTypeTag<wxSizeEvent>; ClassTest; EventArg # wxSizeEvent; EventHandlerTest]'
protbind.cpp:9:2:   required from here
include/wx/event.h:294:18: error: 'wxEvtHandler' is an inaccessible base of 'Test'
         { return static_cast<T *>(p); }
                  ^~~~~~~~~~~~~~~~~~~

Renaming the Host template parameters gets rid of the shadowing errors, but the last one still remains.

[wxtrac]

2016-08-20 07:18:43: tambre (Raul Tambre) uploaded file derivfix.patch (2.4 KiB)

A patch to fix the issue described in the ticket.

[wxtrac]

2016-08-20 07:21:54: tambre (Raul Tambre) commented

---

I moved the Host template out of that template and renamed it to be slightly more meaningful. I'd rather not rename the parameters, as they help with the understanding of what it does.

As for that cast error, it's due to the derived class not being public, thus again not being directly convertible. Using reinterpret_cast should solve that.

I think this should now compile on g++.

[wxtrac]

2016-08-21 16:22:47: @vadz commented

---

Yes, I can confirm that it compiles with g++ now, thanks.

I still don't understand something here... After this change, wxConvertibleTo<> still returns true for the protected/private inheritance -- wasn't the idea that it shouldn't do this? IOW it looks to me like all the changes to wxConvertibleTo<> didn't change anything and it's just that the additions of reinterpret_cast<> suppress the errors now.

I believe reinterpret_cast<> shouldn't be used in wx/event.h and we just need to ensure that wxConvertibleTo<> returns false in this case. Am I missing something?

P.S. For compatibility reasons we might prefer not to touch wxConvertibleTo<> and add some new wxPubliclyInheritsFrom<> instead.

[wxtrac]

2016-08-21 17:28:56: tambre (Raul Tambre) commented

---

The whole point of the change was to fix wxConvertibleTo<> returning false for when the inheritance is protected or private. Technically such inheritance still is convertible, but the static conversion (static_cast<>) is inaccessible. In such cases you'll have to convert using reinterpret_cast<>.

I've added a new template wxStaticallyConvertibleTo<> that checks if D can be converted to B using static_cast<>. This is equivalent to the old wxConvertibleTo<>. I've changed this to be used everywhere where wxConvertibleTo<>'s original behaviour was originally used.

As for wxConvertibleTo<>, I changed switched around the template parameters to be same as the standard library std::is_base_of<>. I've left this to be used in the event.h to fix the original test case given in this ticket. Note that the cast changes are still required due to static conversion not being possible, as explained above.

[wxtrac]

2016-08-21 17:38:49: tambre (Raul Tambre) uploaded file derivfix.2.patch (7.6 KiB)

[wxtrac]

2016-08-22 14:28:44: @vadz commented

---

Replying to [comment:9 tambre]:

The whole point of the change was to fix wxConvertibleTo<> returning false for when the inheritance is protected or private.

Yes, this was my impression as well. But this is not what happens, it still returns true, just use static_assert to check it.

I've added a new template wxStaticallyConvertibleTo<> that checks if D can be converted to B using static_cast<>. This is equivalent to the old wxConvertibleTo<>. I've changed this to be used everywhere where wxConvertibleTo<>'s original behaviour was originally used.

This seems like a good idea, thanks. But did you check that it really did work as intended?

As for wxConvertibleTo<>, I changed switched around the template parameters to be same as the standard library std::is_base_of<>. I've left this to be used in the event.h to fix the original test case given in this ticket. Note that the cast changes are still required due to static conversion not being possible, as explained above.

No, sorry, I don't think this is right. If wx[Statically]ConvertibleTo<> worked correctly, this code shouldn't be compiled in the first place, it's only used when it returns true. Changing it shouldn't be necessary.

[wxtrac]

2016-08-22 14:50:46: tambre (Raul Tambre) commented

---

Replying to [comment:10 vadz]:

Replying to [comment:9 tambre]:

The whole point of the change was to fix wxConvertibleTo<> returning false for when the inheritance is protected or private.

Yes, this was my impression as well. But this is not what happens, it still returns true, just use static_assert to check it.

Now it returns true when inheritance is protected or private, it would previously return false (see wxStaticallyConvertibleTo). I think you might've misunderstood.

I've added a new template wxStaticallyConvertibleTo<> that checks if D can be converted to B using static_cast<>. This is equivalent to the old wxConvertibleTo<>. I've changed this to be used everywhere where wxConvertibleTo<>'s original behaviour was originally used.

This seems like a good idea, thanks. But did you check that it really did work as intended?

Yes. It does work, I tested it using static_assert's.

As for wxConvertibleTo<>, I changed switched around the template parameters to be same as the standard library std::is_base_of<>. I've left this to be used in the event.h to fix the original test case given in this ticket. Note that the cast changes are still required due to static conversion not being possible, as explained above.

No, sorry, I don't think this is right. If wx[Statically]ConvertibleTo<> worked correctly, this code shouldn't be compiled in the first place, it's only used when it returns true. Changing it shouldn't be necessary.

The new wxConvertibleTo<> fixes the test case here along with the cast changes. Note that using wxStaticallyConvertibleTo<> (which is the old behaviour) would cause a compile-time error.

[wxtrac]

2016-09-21 19:35:28: tambre (Raul Tambre) commented

---

Any news on getting this patch merged into master? I would like to have this merged as it would slightly streamline the build process for my application, since it currently relies on the patch attached to this ticket.

[wxtrac]

2016-09-21 22:27:32: @vadz commented

---

Sorry for the delay, I was extremely busy with other things in the beginning of the month and didn't have time for wxWidgets at all.

Now that I get back to this, I think we have a fundamental disagreement explaining why we don't understand each other. If I understand correctly, you want to fix HandlerImpl<T, A, true> to work even when T doesn't inherit from wxEvtHandler publicly. I think this is wrong and we should instead use HandlerImpl<T, A, false> in this case because if T inherits from wxEvtHandler privately (or protectedly), it's as if it didn't inherit from it at all and we definitely don't want to break its encapsulation by using reinterpret_cast<> to pry it open and get wxEvtHandler* from T* anyhow. This seems rather obvious to me, e.g. just consider the case of multiple inheritance when reinterpret_cast<> would just plainly give you a wrong pointer. So I didn't even realize that your intention was to fix it in this way.

Unless I'm still missing something very fundamental, the real solution is, conceptually, to make sure that wxConvertibleTo<D, B> compiles and returns false when B is a non-public base class of D. Can we agree on this?

Also, I previously suggested keeping wxConvertibleTo<> old behaviour for compatibility, but, actually, as it just doesn't compile at all in this case now, I don't see how could making it compile break anything, so I now believe we can just fix it.

Of course, we still can't just exchange the order of its arguments because this would silently break any existing code relying on it...

[wxtrac]

2016-09-21 22:42:27: @vadz changed status from confirmed to accepted

2016-09-21 22:42:27: @vadz commented

~~ IOW I think that this PR is all that we need to do to fix the problem, do you see any problems with it? ~~

Edit: no, clearly this is wrong as now wxConvertibleTo<> just always returns false, sorry. But the basic idea in the comment above is still correct, IMO.

[wxtrac]

2016-09-21 23:54:44: @vadz commented

---

Sorry, I ran out of time and still didn't find any way to detect only public inheritance here. And even C++11 std::is_base_of<> doesn't do it neither, i.e. it returns true for private inheritance as well. Could this test be really impossible to do? I can't believe it, but OTOH I can't find how to do it so far neither...

[wxtrac]

2016-09-26 23:53:39: mmarsan (Manolo) commented

---

I'm not able to find a way to test if a class D derives from class B, returning 0 for non-public inheritance and without using C++11.
But I found a way to do this test in case B is wxEventHandler. I look for void wxEventHandler::UnLink().
It works for all cases, even if D does not derive from B.
I've tested it only with TDM g++ 5.1
I'm not making a patch because I consider next code as a first idea. Here it is:

#!cpp
template <class D, class B>
struct IsPublicDerivedFromwxEvtHandler
{
    // void UnLink() is a member from wxEvtHandler
    typedef void (B::*pToUnlink)();

    template <typename T, T>
    struct stTest;

    template <typename U>
    static char Match( stTest<pToUnlink, &U::Unlink>* );

    // This function also needs to be a template
    template <typename S>
    static int Match(...);

   _//__//__//__//__//__//__//_////
    enum
    {
        value # sizeof( Match<D>(0) )= sizeof(char)
    };
};

IsPublicDerivedFromwxEvtHandler() should be used instead of wxConvertibleTo().

Currently wxConvertibleTo() is used in wx/event.h:335 and wx/weakref.h:22
Because wxEventHandler derives from wxTrackable it likely would be better to add a void Foo() to wxTrackable and change this code to look for void wxTrackable::Foo().

[wxtrac]

2016-10-02 15:19:04: @vadz commented

---

Thanks, I still didn't have time to test this, but I agree that it's a perfectly acceptable solution and I'd be glad to apply a full patch implementing it. I even think we could add some wxEvtHandler::WXSpecialMethodForEventHandlerDetection() just to make this work -- because why not?

I also am wondering why do we actually need to treat wxEvtHandler specially when things work (don't they?) just fine for classes non-deriving from it too. Perhaps all this is not needed at all? But I admit I don't fully understand what's going on with the reference tracking etc in event.cpp.

[wxtrac]

2016-10-08 12:39:02: tambre (Raul Tambre) uploaded file derivfix.3.patch (2.2 KiB)

[wxtrac]

2016-10-08 12:48:01: tambre (Raul Tambre) commented

---

I've adapted the mmarsan's solution with the suggestion from vadz. This works fine on clang++-3.9. Is this patch OK?

[wxtrac]

2016-10-09 19:53:20: mmarsan (Manolo) commented

---

As I wrote, wxConvertibleTo is also used in wx/weakref.h:22 to test if T can be converted to wxTrackable.

Because wxEventHandler derives from wxTrackable, a new function void wxTrackable::WXSpecialMethodForTrackableDetection() should be added to wxTrackable instead. And at struct wxIsPublicDerivedFromwxTrackable check for this new function. Otherwise it would break some code that uses wxWeakRef. I haven't tested it though.

We can leave wxConvertibleTo<> code as is. But change its two calls at wx/event.h:335 and wx/weakref.h:22 with wxIsPublicDerivedFromwxTrackable<>.

[wxtrac]

2016-10-09 20:02:12: mmarsan (Manolo) commented

---

Also, in the case of non-public heritance, the bound handler will not take part of normal wx event chain (no propagation, etc). This should be noted in docs, as now non-public heritance will not be a compiler fail, and the user ignore it.

[wxtrac]

2016-10-10 20:38:45: tambre (Raul Tambre) commented

---

I've attached a new patch.

Unfortunately it seems that compilation will fail if WXSpecialMethodForTrackableDetection() is not public. Exposing this to everything might not be the best idea. Any ideas?

I've also added a documentation entry on how to bind a member function as an event handler. The example demonstrates this on a non-publicly deriving class. Below the example is a note explaining the consequences of non-publicly deriving event handlers. Should I move the explanation about consequences of non-publicly deriving event handlers somewhere else?

I also fixed small 2 typos in documentation while adding the documentation entry.

[wxtrac]

2016-10-10 20:48:55: tambre (Raul Tambre) uploaded file derivfix.4.patch (10.3 KiB)

[wxtrac]

2016-10-11 18:09:41: tambre (Raul Tambre) commented

---

This actually doesn't seem to work. Not sure how I missed that, sorry.

Output on MSVC14 on my project using derivfix.4.patch:

wxWidgets\include\wx/meta/convertible.h(56): error C2247: 'wxTrackable::WXSpecialMethodForTrackableDetection' not accessible because 'MyFrame' uses 'private' to inherit from 'wxFrame'

Are there any alternatives or did I do something wrong myself?

[wxtrac]

2016-10-14 01:44:03: mmarsan (Manolo) commented

---

I asked Bjarne Stroustrup and he gave me a clean an easy way for detecting public-inheritance in C++11:

#!cpp
std::is_base_of<B,D>::value && std::is_convertible<D,B>::value

Brrrr. I was too focused in C++98 and forgot C++11...

Depending on the compiler, we have three cases:
if C++11 is in use (i.e. __cplusplus >= 201103)
Use code above
elseif GCC (some version after 4.??) or Clang (some version after ??) or ???
Use code with wxTrackable::foo() tambre's derivfix.4.patch
else
Use existing wxConvertibleTo

This is not a full solution, but at least narrows the user's issues.

[wxtrac]

2016-10-15 11:52:40: tambre (Raul Tambre) uploaded file derivfix.5.patch (11.3 KiB)

[wxtrac]

2016-10-15 11:54:32: tambre (Raul Tambre) commented

---

Thank you for the suggestions, I've corrected the code to do what you mentioned and have uploaded a new patch. Unfortunately I think wxIsPublicDerivedFromwxTrackable function derivation detection trick will still not work. Would someone be able to confirm it on an older compiler?

[wxtrac]

2016-10-16 21:25:13: mmarsan (Manolo) commented

---

MS being a lot of fun, as usual. VC++10 do has is_base_of and is_convertible. So, what happens if you change

#if __cplusplus >= 201103
...
#elif __GNUC__ >= 4 || _MSC_VER >= 1600

with

#!cpp
#if __cplusplus >= 201103 || (defined(_MSC_VER) && _MSC_VER >= 1600)
...
#elif wxCHECK_GCC_VERSION(4,3)

?

[wxtrac]

2016-10-18 12:37:17: tambre (Raul Tambre) uploaded file derivfix.7.patch (11.4 KiB)

[wxtrac]

2016-10-18 12:41:03: tambre (Raul Tambre) commented

---

I've added a new patch that uses the detection method you've described. This detection does work for at least MSVC14 from my testing.

Do note that still this doesn't compile (at least on MSVC14) due to the wxWeakRef issue mentioned above.

[wxtrac]

2016-10-19 01:55:50: mmarsan (Manolo) commented

---

The only thing I find wrong in your last patch is declaring WXSpecialMethodForTrackableDetection() as private.
In my GCC compiler it must be public or else the test within CHECK_GCC macro returns false, which makes sense in case D is not same as B, but derived from.

This may also be the case for MSVC even when C++11 way is used.

I don't get why you declare it as static .

I don't know why you say it works on your testing but wxWeakRef doesn't. Perhaps on the "private" declaration? Pre-compiled headers maybe?

[wxtrac]

2016-10-25 16:04:41: tambre (Raul Tambre) commented

---

Even when it's set to public and I force it to use the 2nd option (function detection trickery), then GCC 6.2, Clang 3.9 and MSVC14 fail to compile. Do note that the C++11 method will also fail to compile on GCC 6.2, Clang 3.9 and MSVC14.
Could you clarify what GCC version are you using? This shouldn't compile (it certainly doesn't on the newest and latest compilers).

Apologies, seems like there was a typo in my previous comment about it working - it doesn't. Both C++11 and function-detection methods don't work on GCC 6.2, Clang 3.9 or MSVC14 (as mentioned above).
As for declaring it static, this makes the function no longer be required to be public. This seems to further the compilation on all of the compilers I tested on, but they all still fail on wxWeakRef.

For the record, here's the wxWeakRef compiler error from Clang 3.9 using the second method:

In file included from ../src/common/event.cpp:20:
In file included from ../include/wx/wxprec.h:42:
In file included from ../include/wx/wx.h:41:
In file included from ../include/wx/toplevel.h:22:
../include/wx/weakref.h:100:33: error: named bit-field 'Tracked_class_should_inherit_from_wxTrackable' has zero width
                                Tracked_class_should_inherit_from_wxTrackable );
                                ^
../include/wx/weakref.h:99:9: note: in instantiation of member class 'wxAssert_100' requested here
        wxCOMPILE_TIME_ASSERT( wxIsStaticTrackable<TDerived>::value,
        ^
../include/wx/debug.h:442:16: note: expanded from macro 'wxCOMPILE_TIME_ASSERT'
        struct wxMAKE_UNIQUE_ASSERT_NAME { unsigned int msg: expr; }
               ^
../include/wx/debug.h:412:45: note: expanded from macro 'wxMAKE_UNIQUE_ASSERT_NAME'
#define wxMAKE_UNIQUE_ASSERT_NAME           wxMAKE_UNIQUE_NAME(wxAssert_)
                                            ^
../include/wx/cpp.h:75:37: note: expanded from macro 'wxMAKE_UNIQUE_NAME'
#define wxMAKE_UNIQUE_NAME(text)    wxCONCAT_LINE(text)
                                    ^
note: (skipping 1 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
../include/wx/cpp.h:21:5: note: expanded from macro 'wxCONCAT'
    wxCONCAT_HELPER(x1, x2)
    ^
../include/wx/cpp.h:18:37: note: expanded from macro 'wxCONCAT_HELPER'
#define wxCONCAT_HELPER(text, line) text ## line
                                    ^
<scratch space>:37:1: note: expanded from here
wxAssert_100
^
../include/wx/weakref.h:40:15: note: in instantiation of function template specialization
      'wxWeakRef<wxWindow>::Assign<wxWindow>' requested here
        this->Assign(pobj);
              ^
../include/wx/toplevel.h:233:60: note: in instantiation of member function 'wxWeakRef<wxWindow>::wxWeakRef' requested
      here
        { wxWindow *old # GetDefaultItem(); m_winDefaultwin; return old; }
                                                           ^

[wxtrac]

2016-10-26 01:10:57: mmarsan (Manolo) commented

---

The compiler I'm using for tests is TDM-GCC 5.1 32 bits.

For C++11 flag (CXXFLAGS="-std=gnu++11" in my command) this change must be done at convertible.h:
value = std::is_base_of<B,D>::value && std::is_convertible<D*,B*>::value
please, note the is_convertible<D*,B*> pointer usage.
Minimal sample and your 'wxWidgets Bind Sample.cpp' both run well; and the right HandlerImpl<> is deduced.

For not using C++11 the problem comes from wxEvtHandler derives first from wxObject, not from wxTrackable. So the this implicit parameter invalidates the check (detecting the function at wxTrackable).

At this point I do not dare to change the order of inheritance without some advise. Also more SFINAE -if somebody achieve it- seems too hackish.

Vadim, what do you think?

[wxtrac]

2016-10-26 03:11:32: mmarsan (Manolo) commented

---

I've not fully tested, but this seem to work (changes from derivfix.7.patch)
at tracker.h:

#!cpp
public:
    // This is a special method used by wxIsPublicDerivedFromwxTrackable detect
    // if a class is publically derived from wxTrackable. Do not use this for
    // anything else
    void WXSpecialMethodForTrackableDetection();

The 'private' & 'static' thing does not compile, same error as comment #29

at convertible.h:

#!cpp
// Checks if the class is publicly derived from wxTrackable.
class wxTrackable;
template <class D, class B>
struct wxIsPublicDerivedFromwxTrackable
{
    // WXSpecialMethodForTrackableDetection is a special method for this check
    typedef void (wxTrackable::*pToWXSpecialMethodForTrackableDetection)();

    template <typename T, T>
    struct stTest;

    template <typename U>
    static char Match(stTest<pToWXSpecialMethodForTrackableDetection, &U::WXSpecialMethodForTrackableDetection>*);

    // This function also needs to be a template
    template <typename S>
    static int Match(...);

    enum
    {
#if __cplusplus >= 201103 || (defined(_MSC_VER) && _MSC_VER >= 1600)
        // If C++11 is available we use this, as on most compilers it's a
        // built-in and will be evaluated at compile-time
        value = std::is_base_of<B,D>::value && std::is_convertible<D*,B*>::value
#elif wxCHECK_GCC_VERSION(4,3)
        // On slightly old compilers we use the function trick above
        value # sizeof(Match<D>(0))= sizeof(char)
#else
        // On very old compilers we fall back to wxConvertibleTo
        value = wxConvertibleTo<D, B>::value
#endif
    };
};

Other:

• gcc version may be less than 4.3, I'm not sure.
• what other compilers?

[wxtrac]

2016-10-27 16:11:43: tambre (Raul Tambre) uploaded file derivfix.8.patch (5.5 KiB)

[wxtrac]

2016-10-27 16:15:52: tambre (Raul Tambre) commented

---

I have attached a new patch. Tested both C++11 version and the first fallback version on Clang 3.9 and both work without any problems on the latest patch. Thanks for the help with the fix!

I'll try to also test it on my real application using MSVC14 in the next couple days and I'll report back.

[wxtrac]

2016-10-28 17:57:39: tambre (Raul Tambre) commented

---

Works without problems on MSVC14 on my application that uses wxWidgets. Looks good to go to me.

As for the compiler fallback detection, it might make sense to keep the current ones and add more if needed, since problems will only occur if someone tries to use non-public inheritance on older/ancient compilers.

[wxtrac]

2016-11-20 12:49:27: tambre (Raul Tambre) commented

---

I can also confirm that it compiles and works on MSVC15 with this patch applied to master. (Though there is a minor problem where the developer command prompt didn't properly set 64-bit environment variables, but that's a bug in VS2017 itself)

Anything else that needs to be done for this to be accepted into master?

[wxtrac]

2016-11-20 22:35:54: @vadz commented

---

Sorry, I'm feeling pretty dumb here, but I don't understand what's going on here: when do we actually use WXSpecialMethodForTrackableDetection()? It doesn't seem to be used when using C++11 nor when using non-C++11 non-g++-4.3-or-later compiler, is it really just for g++ 4.3+ in C++98 mode? If so, what determines this compiler/version check exactly, i.e. why don't we use this for MSVC versions < 2010 too?

I'm also rather confused by having wxIsPublicDerivedFromwxTrackable<D,B>: what is B here if it's checking whether it's defined from wxTrackable? But then, of course, it actually does seem to check whether it's derived from B (at least in C++11 case, which is the most clear), so isn't the name misleading?

Finally, what exactly does this:

Do note that when your class doesn't publicly derive from wxTrackable somewhere
down the line, then your event handler will @b not take part of the normal
wxWidgets event chain (for example event propagation will not work)

mean? Could you please give an example of what doesn't work and maybe explain why?

TIA!

[wxtrac]

2016-11-21 14:42:49: mmarsan (Manolo) commented

---

WXSpecialMethodForTrackableDetection is only used in the g++4.3 check with the SFINAE thing. As I said in comment 31 other versions and compilers should be added. All that have safe and complete templates stuff. I chose g++4.3 because I know it to work well.

Keeping B is for the cases where that g++4.3 check is not used, i.e. c++11 and wxConvertibleTo.

The event chain... I admit I don't know well all of event internals. But I see a lot of wxEventHandler::GetNextHandler(). This makes me think that if the bound function doesn't derives from wxEvenHandler, GetNextHandler() can't be called...

[wxtrac]

2016-11-21 14:47:41: @vadz commented

---

Replying to [comment:36 mmarsan]:

WXSpecialMethodForTrackableDetection is only used in the g++4.3 check with the SFINAE thing. As I said in comment 31 other versions and compilers should be added. All that have safe and complete templates stuff. I chose g++4.3 because I know it to work well.

What exactly "template stuff" does it rely on? It doesn't look very different from the code currently in wx/meta/convertible.h...

Do we know of any compilers (MSVC 200x?) that don't compile this? If not, we should just enable it by default and wait until we actually run into any troubles with it.

Keeping B is for the cases where that g++4.3 check is not used, i.e. c++11 and wxConvertibleTo.

But what is the meaning of this template? Does it check whether D derives from B or from wxTrackable? The signature and the name are just contradictory right now :-(

The event chain... I admit I don't know well all of event internals. But I see a lot of wxEventHandler::GetNextHandler(). This makes me think that if the bound function doesn't derives from wxEvenHandler, GetNextHandler() can't be called...

This is very different from "event propagation won't work". Event chain is, indeed, specific to wxEvtHandler (and mostly obsolete). Event propagation, e.g. bubbling up the event through the window hierarchy, is not.

[wxtrac]

2016-11-21 16:40:45: mmarsan (Manolo) commented

---

wxConvertibleTo fails at compile time for Bind() for not-publicly-wxTrackable-derived classes. This patch allows compiling such cases.

In case of not public heritance wxIsPublicDerivedFromwxTrackable::value is false, as expected. So Bind() choses the 'functor brach' instead of the wxEventHandler one. That's why I thought event chain may be affected.

The template works with both <D,B>, and its name would have been better wxIsPublicDerivedFrom. But for the second branch (not C++11) the only SFINAE trick I have found out is using a signature for wxTrackable, which is the only class where currently wxWidgets uses this template. So, only in this branch B is not used, wxTrackable is used instead. And that's why such explicit name for the template.

About compilers, I don't know. Borland? Cygwin? MSVC < 1600? What about OSX? I expect others to check this item.

[wxtrac]

2016-11-21 16:46:06: @vadz commented

---

Wait, I definitely can call Bind() with methods of non-wxTrackable-derived classes right now. What do you mean by "wxConvertibleTo fails at compile time" in this case?

And so the real name for this template should be wxIsPubliclyDerivedFromBaseOrTrackableIfCantDetermine ?? I'm having a lot of trouble understanding the logic here :-( I don't even know how did we end up with wxTrackable in the picture when we had started with wxEvtHandler. Could you please describe what are we actually trying to do here in simple words?

Honestly, perhaps we should just forget about this issue, it takes an inordinate amount of time for something which is a really, really minor problem.

Compiler is the least of our issues: Borland is dead, Cygwin is gcc, OS X uses clang.

[wxtrac]

2016-11-21 17:03:05: mmarsan (Manolo) commented

---

Well, read the OP initial comment and take a look at the sample he provides. We're trying to allow binding to a member function of a not publicly wxEventHandler derived class. Currently, compilation of that sample fails, which should not happen.
Fixing it only for wxEventHandler makes compilation of wxWidgets itself to fail. wxTrackable is the one to take account on.

The ideal fix would be to make wxIsConvertibleTo able to compile also for not public heritance for any pair <D,B>. But I didn't find a way, out of C++11.

It's a minor problem? Likely yes, priority of this ticket is low. But OTOH we have a patch, tested with most popular compilers. Just doc's need a little fix.

[wxtrac]

2016-11-21 18:05:41: @vadz commented

---

OK, what about a compromise? Implement the clean and clear solution with C++11 and forget about the rest.

Would this be acceptable?

[wxtrac]

2016-11-21 19:23:54: mmarsan (Manolo) commented

---

A C++11 only solution must be advised in the doc's (compiling both wxWidgets and app in C++11 mode). I see no other problem.

[wxtrac]

2016-11-22 02:03:47: @vadz commented

---

Could you (or @tambre) please update the patch to do just this? TIA!

[wxtrac]

2016-11-22 06:40:25: tambre (Raul Tambre) commented

---

I'm willing to update the patch, though it might take until the weekend.

As from what I understand, we're dropping support for all non-C++11 compilers and using only the C++11 variant that's in the latest patch?

[wxtrac]

2016-11-22 17:53:45: @vadz commented

---

Yes, I think it's better to do the simple and obviously correct thing for C++11 and leave the things unchanged for C++98 rather than introducing complicated and not necessarily correct workarounds for the latter.

TIA!

[wxtrac]

2016-11-25 17:52:08: tambre (Raul Tambre) uploaded file derivfix.9.patch (5.4 KiB)

[wxtrac]

2016-11-25 17:52:41: tambre (Raul Tambre) commented

---

I've attached a new patch that hopefully addresses the problems that were brought up above.
Please review and let me know if there's still anything that needs to be improved.

[wxtrac]

2016-11-28 14:56:54: @vadz commented

---

Thanks, I've applied the latest patch with 2 changes:

1. Don't exchange the order of arguments of wxIsPubliclyDerived compared to wxConvertibleTo. IMO it's confusing and doesn't read correctly (it's D that is publicly derived from B, not vice versa).
2. Didn't apply the documentation changes because I am still not sure what does "not take part of the normal wxWidgets event chain" mean and I don't think that saying that the event propagation won't in this case is correct.

I've created a PR with this patch and if/when it passes the CI tests, I'll merge it.

Thanks again!

[wxtrac]

2016-11-29 19:32:51: @wxtrac changed status from accepted to closed

2016-11-29 19:32:51: @wxtrac changed resolution from ** to fixed

2016-11-29 19:32:51: @wxtrac commented

In 5551932:
Allow using Bind() with non-public inheritance in C++11 code

Using Bind() with a method of the class deriving from wxEvtHandler
non-publicly used to result in a compile-time error, but at least with C++11
we can detect this case and allow the code to compile.

Closes #17623.

[wxtrac]

2016-11-29 19:55:23: tambre (Raul Tambre) commented

---

The documentation changes seem to be missing. Should probably be added too, with the changes mentioned in comment:46.

[wxtrac]

2016-11-30 15:29:59: @vadz commented

---

Yes, I mentioned that I didn't apply the documentation changes (point 2 of comment:47) because they didn't seem clear enough (and maybe not even correct) to me.

[wxtrac]

2016-12-03 18:34:06: tambre (Raul Tambre) commented

---

I opened a pull request (#362) to add the documentation changes, that were left out.