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