Re: Avoid unused value (src/fe_utils/print.c)
От | Ranier Vilela |
---|---|
Тема | Re: Avoid unused value (src/fe_utils/print.c) |
Дата | |
Msg-id | CAEudQArAPOpbNpW2MSOfuDJ7QwkeNCNeR9zDW0iOzapfc_m95w@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: Avoid unused value (src/fe_utils/print.c) (Alexander Lakhin <exclusion@gmail.com>) |
Список | pgsql-hackers |
Em sex., 30 de jun. de 2023 às 13:00, Alexander Lakhin <exclusion@gmail.com> escreveu:
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.
Hi Alexander,
I think that the question that should be answered before moving forward
here is: what this discussion is expected to result in?
I hope to make the function more readable and maintainable.
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).
if (cont->opt->stop_table)
477 {
478 printTableFooter *footers = footers_with_default(cont);
479
480 if (!opt_tuples_only && footers != NULL && !cancel_pressed)
481 {
482 printTableFooter *f;
483
484 for (f = footers; f; f = f->next)
485 {
486 if (need_recordsep)
487 {
488 print_separator(cont->opt->recordSep, fout);
CID 1512766 (#1 of 1): Unused value (UNUSED_VALUE)assigned_value: Assigning value false to need_recordsep here, but that stored value is overwritten before it can be used.
489 need_recordsep = false;
490 }
491 fputs(f->data, fout);
value_overwrite: Overwriting previous write to need_recordsep with value true.
492 need_recordsep = true;
493 }
494 }
495
Otherwise, if the goal is to do review the code and make it cleaner, then
why not get rid of "if (need_recordsep)" there?
I don't agree with removing this line, as it is essential to print the separators, in the footers.
best regards,
Ranier Vilela
В списке pgsql-hackers по дате отправления: