Re: [lmi] [lmi-commits] master 78a4de0 5/7: Enable '-Wnull-dereference'; fix the issues it flags

[Vadim Zeitlin]

On Fri, 22 Mar 2019 05:53:36 -0400 (EDT) Greg Chicares <address@hidden> wrote:

GC> branch: master
GC> commit 78a4de074070eae5daf69dfd397297765d8a3793
GC> Author: Gregory W. Chicares <address@hidden>
GC> Commit: Gregory W. Chicares <address@hidden>
GC> 
GC>     Enable '-Wnull-dereference'; fix the issues it flags

 Hello,

 What was the actual issue?

GC> diff --git a/input_sequence_entry.cpp b/input_sequence_entry.cpp
GC> index 902ef9b..34d1ce9 100644
GC> --- a/input_sequence_entry.cpp
GC> +++ b/input_sequence_entry.cpp
GC> @@ -806,7 +806,7 @@ void InputSequenceEditor::remove_row(int row)
GC>          int index = row * Col_Max;
GC>          wxWindow* win = sizer_->GetItem(index)->GetWindow();
GC>          sizer_->Detach(index);
GC> -        win->Destroy();
GC> +        win && win->Destroy();
GC>          }

 FWIW I really don't think it's a good idea to write things like this, if
GetWindow() ever returns null here, it would indicate a serious bug and it
shouldn't be silently ignored. I'd rather use LMI_ASSERT() here if a check
is needed.

 BTW, the call to wxSizer::Detach() here is superfluous, a window would
detach itself from the containing sizer in any case when it's destroyed.
I'm not sure if it's worth keeping this call for clarity but I'd probably
remove it.

 And, to finish with snippet, I also find it rather confusing that "index"
is initialized inside the loop when it's actually constant, I think a
comment explaining that we do it like this because the indices get adjusted
as items are deleted would be helpful here.

GC>      redo_layout();
GC> diff --git a/main_wx_test.cpp b/main_wx_test.cpp
GC> index a4d5a3d..25ee2a3 100644
GC> --- a/main_wx_test.cpp
GC> +++ b/main_wx_test.cpp
GC> @@ -591,7 +591,11 @@ wxWindow* 
wx_test_focus_controller_child(MvcController& dialog, char const* name
GC>      // until we reach it.
GC>      for(wxWindow* maybe_page = w;;)
GC>          {
GC> -        wxWindow* const maybe_book = maybe_page->GetParent();
GC> +        wxWindow* const maybe_book =
GC> +            (maybe_page && maybe_page->GetParent())
GC> +            ? maybe_page->GetParent()
GC> +            : nullptr
GC> +            ;

 This check is again very suspicious because a child window is guaranteed
to always have its top level parent (which is "dialog" here) in its
ancestors chain, so if it fails, it would indicate that something is
catastrophically wrong. I would again prefer to use LMI_ASSERT() here.

 Regards,
VZ