Re: [lmi] Class wx_table_generator

[Greg Chicares]

On 2018-04-30 23:48, Vadim Zeitlin wrote:
> On Mon, 30 Apr 2018 23:24:29 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> In output_header(), we return early if we're only measuring:
> GC>     case oe_only_measure:
> GC>         *pos_y += max_header_lines_ * row_height_;
> GC>         return;
> GC> but in that case the special code for bold headers
> GC>     wxDCFontChanger header_font_setter(dc_);
> GC> isn't taken into account. Does that work by design (i.e., because making a
> GC> font bold with
> GC>     dc_.GetFont().Bold();
> GC> doesn't change its dc_.GetCharHeight()), or by accident (i.e., because we
> GC> always happen to have enough space to accommodate a greater font height)?
> 
>  It is intentional, but definitely merits a comment, which I didn't write.
> The fact is that even when drawing, we will always consume exactly as much
> space as returned above, i.e. the height of the font is not taken into
> account at all because do_output_values() increments the vertical position
> by (fixed) row_height_ and not by dc_.GetCharHeight() or something similar.

char_height_ and row_height_ do differ in the ctor-initializer:
    ,char_height_(dc_.GetCharHeight())
    ,row_height_((4 * char_height_ + 2) / 3) // Arbitrarily use 1.333 line 
spacing.
but later they may not:

  void wx_table_generator::use_condensed_style()
  {
      row_height_ = char_height_;
      draw_separators_ = false;
[But in practice that doesn't matter--today--because:]
      use_bold_headers_ = false;
  }

Nevertheless, I'll add a comment.

> GC> Where we use this clipper:
> GC>     wxDCClipper clip
> GC>         (dc_
> GC>         ,wxRect
> GC>             {wxPoint{x, y_top}
> GC>             ,wxSize{ci.col_width() - column_margin(), row_height_}
> GC>             }
> GC>         );
> GC> what happens if 'ci.col_width() - column_margin()' is negative? Does the
> GC> wxDCClipper code force the negative to zero, so that everything is 
> clipped?
> GC> Or is that an error that we should prevent, by forcing zero ourselves?
> 
>  I honestly have no idea what happens when setting a clipping region with
> negative width (I could test it tomorrow, but today I'd just like to send
> this reply to answer your question without delay), but I think it would be
> better to ensure that it doesn't happen in this code, which we (well, I)
> indeed don't do right now, thanks for noticing this.

No need to research what it does--we'll guard against it.

>  In fact, the code in do_compute_column_widths() should ensure that the
> column width is strictly greater than the column margin,

Yes, indeed--I know exactly what you mean, and that's what led me to ask.

> but I'm not really
> sure even of this any more... (I'm afraid that while making this code more
> clear for you, you made it correspondingly less clear for me, notably the
> use of column_margin() for accessing the value and column_margin_ to modify
> it makes things very confusing IMHO). Assuming this is still the case, we
> just need to check somewhere that the column initial width wasn't smaller
> than column margin, i.e. 1em. I think it would be better to leave you to
> add this check to avoid conflicts with your other changes, but please let
> me know if you'd like me to do it instead.

I'll take care of everything. I now perceive the unity and conflict of
opposites, so where now we're witnessing the transformation of quantitative
into qualitative changes, soon the negation of the negation will manifest
itself and all will be rendered clear to both of us. We have nothing to
lose but our confusion.

> GC> But you use at() very often. Do you prefer at() to operator[] in all 
> cases,
> GC> because it replaces a segfault with orderly termination?
> 
>  Yes (except that an exception doesn't necessarily lead to termination,
> although it might). I think the optimization of not checking the index
> offered by operator[] can almost never be really justified.
Interesting. I pushed a simple timing test:
  git push origin odd/vector-at-speed
and here are the reformatted results for
 -O3, vs.
 -O0 with libstdc++ debug mode...

/opt/lmi/src/lmi[0]$make unit_tests unit_test_targets=sandbox_test.exe          
    

39 milliseconds with at()
12 milliseconds with []
12 milliseconds with ranged-for

/opt/lmi/src/lmi[0]$make build_type=safestdlib unit_tests 
unit_test_targets=sandbox_test.exe      

478  milliseconds with at()
598  milliseconds with []
2848 milliseconds with ranged-for

Optimized results: at() does take longer, which surprised me a little
because I thought the compiler might optimize its overhead away. But
the test does so many indexing operations that the time difference
would probably be slight if we actually did anything interesting with
the indexed elements--this test just accumulates them.

STL-debug mode: whatever this mode does, it's obviously a lot more
than just checking index bounds. It's curious that at() outperforms
operator[], and that both far outperform a ranged-for loop.

I don't see this evidence as weighty in any case. I'll probably
continue to use operator[] as my personal default in insurance
calculations, where I really want free speed. But I won't object to
your practice in code you've written, which generally does complex
graphical operations where at()'s cost is relatively negligible.