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

[Vadim Zeitlin]

On Fri, 22 Mar 2019 12:49:37 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2019-03-22 11:28, Vadim Zeitlin wrote:
GC> > On Fri, 22 Mar 2019 05:53:36 -0400 (EDT) Greg Chicares <address@hidden> 
wrote:
GC> > 
GC> > GC> branch: master
GC> > GC> commit 78a4de074070eae5daf69dfd397297765d8a3793
GC> > GC> Author: Gregory W. Chicares <address@hidden>
GC> > GC> Commit: Gregory W. Chicares <address@hidden>
GC> > GC> 
GC> > GC>     Enable '-Wnull-dereference'; fix the issues it flags
GC> 
GC> Updated as discussed below:
GC> 
GC> d86e3d94 (HEAD -> master, origin/master, origin/HEAD) Improve some 
'-Wnull-dereference' workarounds

 Thanks!

GC> Here, as (especially) with '-Wswitch-enum':
GC>   ec7be842 Enable '-Wswitch-enum'; fix the issues it flags

 Yes, I understand what -Wswitch-enum does and find it very valuable, so I
don't have any questions about it, I was only wondering about the
-Wnull-dereference one.

GC> >  And, to finish with snippet, I also find it rather confusing that "index"
GC> > is initialized inside the loop when it's actually constant, I think a
GC> > comment explaining that we do it like this because the indices get 
adjusted
GC> > as items are deleted would be helpful here.
GC> 
GC> I tried to write such a comment, but couldn't: after staring at this
GC> code for a couple minutes, I can't even say what it does.

 I did stare at it for a bit, but I do understand what it does now.

GC> The control statement is
GC>     for(int i = 0; i < Col_Max; ++i)
GC> but the loop variable 'i' isn't referenced in the loop body. Here:
GC>         int index = row * Col_Max;
GC> I don't see how 'index' gets adjusted inside this loop, so I can't
GC> see why hoisting this line thus:
GC> 
GC> +    int const index = row * Col_Max;
GC>      for(int i = 0; i < Col_Max; ++i)
GC>          {
GC> -        int index = row * Col_Max;
GC> 
GC> would change the meaning.

 It wouldn't, this was my point when I wrote "it's actually constant"
above.

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

 Here is my attempt:

        // Note that the index here is constant and always refers to
        // the first window in the given row: as the indices of the
        // subsequent elements adjust, by decreasing by one, when we
        // delete this index, repeatedly deleting Col_Max elements at
        // this position results in deleting the entire row contents.

 Does this help?
VZ