Hi The change looks good to me.
On Wed, May 13, 2020 at 11:44 AM Dave Reisner <dreis...@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