[pacman-dev] [PATCH] Avoid depending on side effects in assert(...) expressions

[Anatol Pomozov]

Hi

The change looks good to me.

On Wed, May 13, 2020 at 11:44 AM Dave Reisner <dreisner at archlinux.org> wrote:
>
> When building with -DNDEBUG, assert statements are compiled out to
> no-ops. Thus, we can't depend on assignments or other computations
> occurring inside the assert().
> ---
> It's perhaps worth mentioning that nowhere else in the ALPM codebase
> do we use assert().

I quite like the idea of defensive programming. This is something that
I learnt the hard way when I was working with chips firmware.
So I often add additional checks across the codebase and it saves me
time during active phase of development/refactoring.

>  src/pacman/callback.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/src/pacman/callback.c b/src/pacman/callback.c
> index 25909e02..4240a779 100644
> --- a/src/pacman/callback.c
> +++ b/src/pacman/callback.c
> @@ -862,12 +862,14 @@ static void dload_progress_event(const char *filename, alpm_download_event_progr
>         int64_t curr_time = get_time_ms();
>         double last_chunk_rate;
>         int64_t timediff;
> +       bool ok;
>
>         if(!dload_progressbar_enabled()) {
>                 return;
>         }
>
> -       assert(find_bar_for_filename(filename, &index, &bar));
> +       ok = find_bar_for_filename(filename, &index, &bar);
> +       assert(ok);

A bit of context for this assert(). Both "progress" and "complete"
events should always have a corresponding "init" event where
progressbar structure is initialized.

If callback.c received a "progress" event for a non-existent
progressbar then it is a programming error.

>
>         /* compute current average values */
>         timediff = curr_time - bar->sync_time;
> @@ -902,12 +904,14 @@ static void dload_complete_event(const char *filename, alpm_download_event_compl
>         int index;
>         struct pacman_progress_bar *bar;
>         int64_t timediff;
> +       bool ok;
>
>         if(!dload_progressbar_enabled()) {
>                 return;
>         }
>
> -       assert(find_bar_for_filename(filename, &index, &bar));
> +       ok = find_bar_for_filename(filename, &index, &bar);
> +       assert(ok);
>         bar->completed = true;
>
>         /* This may not have been initialized if the download finished before
> --
> 2.26.2