On 11/21, Tim Rühsen wrote:
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.

I can't recollect how right now. But I remember triggering that assertion a large number of times when I was adding a new option to Wget to for the first time. While coming to grips with the code base, I'd often make a mistake somewhere in one of the three different places which you have to edit to add a new option. This assertion would always trigger and let me know that I'm doing something very wrong.

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.

A well thought of message replaces all assertions that need to remain in production code. Some assertions are meant to make the code fail instantly when some logic doesn't add up. For example the assertion I added at progress.c:1180 should never be available on end-user systems. It makes Wget fail instantly in certain scenarios when it would otherwise just continue downloading. However, as a developer, I'm interested in knowing immediately when that assertion fails. Because it implies that the length of the progress bar being drawn exceeded the size of my screen. This means there's an error in the logic in create_image() which needs to be debugged. Such are the scenarios where a developer wants an assertion, not a debug message. Because I *want* Wget to crash. But in a lot of cases in the Wget codebase, assertions have been used instead of error messages. I've been looking at those recently and over the next week will try to remove quite a few.


--
Thanking You,
Darshit Shah

Attachment: pgpaNVjm85616.pgp
Description: PGP signature

Reply via email to