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

[Greg Chicares]

On 2019-03-22 11:28, Vadim Zeitlin wrote:
> 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

Updated as discussed below:

d86e3d94 (HEAD -> master, origin/master, origin/HEAD) Improve some 
'-Wnull-dereference' workarounds

To compare against a recent original preceding 78a4de0, to ensure that no
random artifact has been introduced by adding and reverting workarounds:

git diff d86e3d94 784d5524 -- input_sequence_entry.cpp main_wx_test.cpp

>  What was the actual issue?

I observed no issue myself.

Here, as (especially) with '-Wswitch-enum':
  ec7be842 Enable '-Wswitch-enum'; fix the issues it flags
my goal was to stop enabling these diagnostics casually from time to time
(and then manually dismissing the old ones that seem meaningless, but greatly
regretting that I hadn't caught and new and meaningful ones earlier), and
instead systematically to "fix" the known issues that may seem harmless, in
order to enable the warnings generally (so that new violations will be
flagged immediately). IOW, I was treating these diagnostics as presumptively
valid, without evidence of actual harm. And I think this was the right thing
to do, although some of my "fixes" may have been suboptimal.

> 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.

Done in d86e3d94.

>  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.

I've added a comment to that effect. I hesitate to remove any line that
Vaclav wrote except with careful testing, and I don't want to make time
to give this much thought, at least not now.

>  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.

I tried to write such a comment, but couldn't: after staring at this
code for a couple minutes, I can't even say what it does. The control
statement is
    for(int i = 0; i < Col_Max; ++i)
but the loop variable 'i' isn't referenced in the loop body. Here:
        int index = row * Col_Max;
I don't see how 'index' gets adjusted inside this loop, so I can't
see why hoisting this line thus:

+    int const index = row * Col_Max;
     for(int i = 0; i < Col_Max; ++i)
         {
-        int index = row * Col_Max;

would change the meaning.

But if you want to write a comment explaining this, I'll gladly
commit it.

> 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.

Done in d86e3d94.