Hello Karina,
30.06.2023 17:25, Karina Litskevich wrote:
> Hi,
>
> Alexander wrote:
>
>> It also aligns the code with print_unaligned_vertical(), but I can't see why
>> need_recordsep = true;
>> is a no-op here (scan-build dislikes only need_recordsep = false;).
>> I suspect that removing that line will change the behaviour in cases when
>> need_recordsep = false after the loop "print cells" and the loop
>> "for (footers)" is executed.
> As I understand cont->cells is supoused to have all cont->ncolumns * cont->nrows
> entries filled so the loop "print cells" always assigns need_recordsep = true,
> except when there are no cells at all or cancel_pressed == true.
> If cancel_pressed == true then footers are not printed. So to have
> need_recordsep == false before the loop "for (footers)" table should be empty,
> and need_recordsep should be false before the loop "print cells". It can only
> be false there when cont->opt->start_table == true and opt_tuples_only == true
> so that headers are not printed. But when opt_tuples_only == true footers are
> not printed either.
>
> So technically removing "need_recordsep = true;" won't change the outcome. But
> it's not obvious, so I also have doubts about removing this line. If someday
> print options are changed, for example to support printing footers and not
> printing headers, or anything else changes in this function, the output might
> be unexpected with this line removed.
I think that the question that should be answered before moving forward
here is: what this discussion is expected to result in?
If the goal is to avoid an unused value to make Coverity/clang`s scan-build
a little happier, then maybe just don't touch other line, that is not
recognized as dead (at least by scan-build; I wonder what Coverity says
about that line).
Otherwise, if the goal is to do review the code and make it cleaner, then
why not get rid of "if (need_recordsep)" there?
Best regards,
Alexander
