Re: [lmi] How InputSequenceEditor::remove_row() works [Was: master 78a4de0 5/7: Enable '-Wnull-dereference'; fix the issues it flags]

[Vadim Zeitlin]

On Fri, 22 Mar 2019 21:08:27 +0000 Greg Chicares <address@hidden> wrote:

GC> Here's how I understand it now. These enumerators:
GC> 
GC> enum Col 
{Col_Value,Col_From,Col_DurationMode,Col_DurationNum,Col_Then,Col_Remove,Col_Add,Col_Max};
GC> values:   0         1        2                3               4        5    
      6       7
GC> 
GC> correspond to this example:
GC> 
GC>         Col_Value       Col_From   Col_DurationMode 
Col_DurationNum,Col_Then,Col_Remove,Col_Add,Col_Max
GC>     //  Employee payment:
GC>     //    [   0]  from issue date until [year]      [ 5]            , then  
 [REMOVE]   [ADD]   [nothing]
GC>     //    [1000]  from   year 5   until [year]      [10]            , then  
 [REMOVE]   [ADD]   [nothing]
GC>     //    [   0]  from   year 10  until [ age]      [70]            , then  
 [REMOVE]   [ADD]   [nothing]
GC>     //    [   0]  from   age 70   until [maturity]                  .
GC> 
GC> So we have an Nx7 array (4x7 in the example above), whose index-origin-zero 
indices are
GC>    0  1  2  3  4  5  6  7
GC>    8  9 10 11 12 13 14 15
GC>   16 17 18 19 20 21 22 23
GC>   24 25 26 27 28 29 30 31

 I think you've already realized this, but while the explanation above is
correct, the diagram is not as it has 8 elements in each row and not 7.

GC> Or maybe I've misunderstood Max_Col, and it's really not an "element",
GC> but a "one-past-the-end" sentinel of measure zero...

 Yes, indeed. It's a common (and IMHO useful) convention to defined XXX_Max
as the last element of XXX enum just to have a constant corresponding to
the number of enum elements.

GC> in which case we'd have
GC> 
GC>    0  1  2  3  4  5  6
GC>    7  8  9 10 11 12 13
GC>   14 15 16 17 18 19 20
GC>   21 22 23 24 25 26 27
GC> 
GC> and the offset of 14 calculated above would be correct.

 I do think it's correct.

GC> I'd point out how trivial this would be in APL, but I promised myself I'd
GC> hit 'Send' by the time the complete test battery completed, so...sending.

 I don't believe that even the power of APL would help to deal with the
poor wxSizer API here. The real problem is that it doesn't provide
something like erase(begin, end) method that would allow doing what this
code does without using an explicit loop and, more generally, that it uses
indices instead of iterators. I think it's too late to change the latter,
but I think it would be generally useful to do something about the former.
Unfortunately it's not as simple as just adding another Remove() overload
because we need to destroy the window as well, but adding Destroy() family
of methods similar to the existing Remove/Detach ones and including an
overload working with a range could be a good solution.

 And then the lmi code could become just

        int index = row * Col_Max;
        sizer_->Destroy(index, index + Col_Max);

which would be much simpler to understand.

 What do you think?
VZ