Re: [Bug-wget] Disable assertions by default

[Tim Ruehsen]

> Let's get this patch through first and others to handle the old assertions
> can flow in over the next week.

Yes, looks good to me. Go push it.

More comments below.

Tim

On Friday 21 November 2014 15:46:36 Darshit Shah wrote:
> On 11/21, Tim Rühsen wrote:
> >On Friday 21 November 2014 13:19:18 Darshit Shah wrote:
> >> On 11/20, Ángel González wrote:
> >> >On 20/11/14 15:29, Darshit Shah wrote:
> >> >>--- a/src/progress.c
> >> >>+++ b/src/progress.c
> >> >>@@ -992,6 +992,7 @@ create_image (struct bar_progress *bp, double
> >> >>dl_total_time, bool done)>>
> >> >>
> >> >>      {
> >> >>      
> >> >>        int percentage = 100.0 * size / bp->total_length;
> >> >>        assert (percentage<= 100);
> >> >>
> >> >>+      assert (false);
> >> >>
> >> >>        if (percentage<  100)
> >> >>        
> >> >>          sprintf (p, "%3d%%", percentage);
> >> >>
> >> >>-- 2.1.3
> >> >
> >> >Surely you didn't want to include this :)
> >> 
> >> Shit! No, that was meant to be for my own debugging purposes. I was
> >> trying
> >> to see if I could induce an assertion failure. Good thing the patch goes
> >> through review first.
> >> 
> >> I've fixed this in the attached patch.
> >
> >Just a comment.
> >In case the assertions are disabled, there still should be a check and a
> >WARNING message. It should inform the user that something weird happened
> >and the mailing list should be informed. Wget should try to continue if
> >possible. We just had the report that an assertion occurred after hours
> >(and many GB) of downloading and Wget just stopped... It was just one file
> >out of thousands that would have been skipped...
> 
> I agree. We should add more logging and a warning message for when a file
> cannot be downloaded. And for such recursive cases, the same information
> should be displayed at the end of the operation too. This is currently
> missing.
> >IMHO. we should have something like this ASAP. And having this, we also
> >might get rid of assertions completely. That could make this patch
> >obsolete.
> I think we should not get rid of assertions. Some of the assertions, like
> the one at init.c:819 or progress.c:1180 are not for handling run-time
> errors but for intimating future developers about a logical error
> immediately. Such checks need to remain in the development code, but is
> worthless in production code.

How can you make sure that a developer runs into the assert at init.c:819 ?
I guess the only sure way to check the 'commands' array is by calling 
test_commands_sorted(). Only a 'make check' does it.

> Which is why I believe that assertions should remain. Just not all the ones
> we currently have.

Some assertions surely *can* remain. What I try to say is: we do not need 
them. A well thought of message / action can always replace an assertion.

Tim

signature.asc
Description: This is a digitally signed message part.