Re: [lmi] Event handling modernization

[Vadim Zeitlin]

On Wed, 30 Mar 2016 23:45:48 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2014-11-30 01:04, Vadim Zeitlin wrote:
GC> [...]
GC> >  The 0002-Don-t-count-all-MDI-children-just-check-if-there-is-.patch one
GC> > addresses my urge to avoid performing unnecessary actions: we don't really
GC> > need the count of MDI children frame here at all, so why bother counting
GC> > all of them (even if "all" is probably virtually always just a few) when 
we
GC> > can stop after just the second one? Subsidiarily, why count them at all if
GC> > we don't have any active child anyhow.
GC> 
GC> This gives me pause:
GC> 
GC> 
http://docs.wxwidgets.org/trunk/classwx_m_d_i_parent_frame.html#a981df3eae1daa82772fe10e3bcba7215
GC> | virtual wxMDIChildFrame* wxMDIParentFrame::GetActiveChild() const
GC> | Returns a pointer to the active MDI child, if there is one.
GC> | If there are any children at all this function returns a non-NULL pointer.
GC> 
GC> I take "any...at all" to mean that if the parent frame has no MDI children,
GC> but does have a child window of some other type, then this function returns
GC> a non-NULL wxMDIChildFrame* that must point to an object that is not 
actually
GC> a wxMDIChildFrame.

 I'm really not sure how can the above be interpreted in this way, the
function declared as returning wxMDIChildFrame* definitely can't return a
(non-null) pointer to something that is not a wxMDIChildFrame. The text
above just emphasizes that if there are any [MDI; implicitly implied]
children, then one of them will be active, so there is no need to test that
it returns a non-null pointer if you know that any [MDI] children exist.

 I think it's reasonable to expect that wxWidgets at least tries to not
invoke undefined behaviour and any reasonable interpretation of the
documentation should be compatible with this general principle.

GC> Maybe I'm reading too much into this, but it worries me:
GC> wouldn't such a pointer be likely to be misused?

 To put it mildly, such pointer could *only* be misused, so there can be
absolutely no sane reason to ever return it and, of course, wxWidgets
doesn't do it.

GC> I'll just rule that out, at no real cost, thus:
GC>      wxMDIChildFrame* child_frame = frame_->GetActiveChild();
GC> -    if(child_frame)
GC> +    if(dynamic_cast<wxMDIChildFrame*>(child_frame))

 This dynamic_cast is totally unnecessary.

GC> Since I rewrote this in order to understand it, let me also ask...here:
GC> 
GC> 
https://github.com/vadz/lmi/commit/461ba86312ed5409237652d19309a3b8f28ddeaa.patch
GC> +        for(wxWindowList::const_iterator i = wl.begin(); i != wl.end(); 
++i)
GC> +            {
GC> +            wxMDIChildFrame* const child = 
dynamic_cast<wxMDIChildFrame*>(*i);
GC> +            if(child && child != child_frame)
GC> 
GC> Why
GC>   wxMDIChildFrame* const child // the thing-pointed-to cannot be changed
GC> instead of
GC>   wxMDIChildFrame const* child // the pointer cannot be changed
GC> ?

 Err, sorry, the comments are exchanged. In my code, it's the pointer that
is const and this indeed was a conscious and deliberate choice: I declare
all local variables that are not going to change "const" as I believe that
this helps a lot with reading and understanding the code. In fact, I'd go
as far as avoiding non-const local variables if it can be done without any
unreasonable contortions.

GC> Anyway, I'll commit
GC>   wxMDIChildFrame const*const child // nothing can be changed
GC> because I don't see any argument against it.

 True, wxMDIChildFrame itself could be const too, but it's all but hopeless
to work with "wxWindow const*" in wxWidgets API as almost all the functions
require non-const pointers, so I don't even try to do it any more.

 Regards,
VZ