Re: [lmi] [lmi-commits] master a889ef0 2/8: Assert some preconditions

[Greg Chicares]

On 2018-08-07 09:52, Vadim Zeitlin wrote:
> On Mon,  6 Aug 2018 18:36:23 -0400 (EDT) Greg Chicares <address@hidden> wrote:
[...]
> GC>     Assert some preconditions
> GC>     
> GC>     Added assertions that were not observed to fail in fairly extensive
> GC>     testing, though it's not necessarily obvious whether the precondition
> GC>     should be
> GC>       index <  container.size()
> GC>     or
> GC>       index <= container.size()
> GC>     in each particular case.

Thanks for looking them over and pointing out one that was too weak (below).

> GC> @@ -226,6 +226,7 @@ void wx_table_generator::output_highlighted_cell
> GC>      ,std::string const& value
> GC>      )
> GC>  {
> GC> +    LMI_ASSERT(column < all_columns().size());
> GC>      if(all_columns().at(column).is_hidden())
> GC>          {
> GC>          return;
> 
>  I don't this precondition is really useful as the call to at() with the
> "column" argument already asserts the same precondition almost as
> explicitly.

Because I'm cross-compiling msw binaries and running them in 'wine',
I have no usable debugger. When at() fails, I know only that a bounds
error occurred somewhere. When LMI_ASSERT fails, I know where the
failure occurred: i.e., it gives me the information I need.

> However if you prefer to write it like this, then, IMO, it
> would make sense to replace the call to at() with operator[].

This function no longer calls at(), but the asserted precondition
remains, which is exactly what I want.

Even if at() were still called here, I'd see no need to replace it
with operator[], because at() is cheap, while replacing it requires
thought and opens up the possibility of introducing a new defect.
I don't think of the assertion as an adjunct to at() that just makes
it more informative--instead, I see it as enforcing a contract,
which incidentally happened to guard a call to at().

> GC> @@ -334,6 +335,7 @@ int wx_table_generator::separator_line_height() const
> GC>  
> GC>  wxRect wx_table_generator::text_rect(std::size_t column, int y) const
> GC>  {
> GC> +    LMI_ASSERT(column <= all_columns().size());
> GC>      wxRect z = cell_rect(column, y).Deflate(dc().GetCharWidth(), 0);
> GC>      z.Offset(0, (row_height_ - char_height_)/2);
> GC>      return z;
> 
>  This precondition is too lax, "column" must be a valid index here as it's
> passed to cell_rect().

Okay, thanks, I'll change it.