[Edbrowse-dev] if then else

[Edbrowse-dev] if then else

[Karl Dahlke]

Ok, my last push changed over 400 lines of code.
Really?
Yes, and all bug fixes; some serious,
serious enough to cause a segfault.
What happened?

My first mistake was in jsrt.
I didn't include any tests for the URL class.
That class is very special and very complicated, over a thousand lines of code.
With no regression tests, bugs could creep in and nobody would know.
And they did.
My latest push adds tests for this class, so this won't happen again.
See jsrt line 536.

I'm not going to describe all the bugs, but here is a funny one.
We had, for a long time, code like this:

if(condition)
do this
else
do that

But no error legs in js was leading to nulls and segfaults,
so we put in error legs all over the place.
This we needed to do, and thanks to everyone for your help.
There were so many, the process was rather mechanical,
and understandably so.
Just get er done.
Thus the above code became:

if(condition)
if(do this and it fails) {
js fail
return
}
else
if(do that and it fails) {
js fail
return
}

If you've studied formal language theory and compilers,
then you know this is the region of the C grammar that is ambiguous.
But the compiler doesn't tell you that,
it just picks a derivation and marches on.
Which one?
Not the one we expect.
I changed the code as below and several bugs went away.

if(condition) {
if(do this and it fails) {
js fail
return
}
} else {
if(do that and it fails) {
js fail
return
}
}

There were quite a few blocks like this.
So there you go - bitten by the if else ambiguity of C.

There were other bugs too, dealing with string allocation
and a malformed url constructor.
I think they are fixed now; jsrt passes anyways.
I'm glad I cleaned all this up before release 3.5.1.

Karl Dahlke

Re: [Edbrowse-dev] if then else

[Adam Thompson]

[-- Attachment #1: Type: text/plain, Size: 2605 bytes --]

On Tue, Apr 22, 2014 at 12:34:26PM +0000, Karl Dahlke wrote:
> Ok, my last push changed over 400 lines of code.
> Really?
> Yes, and all bug fixes; some serious,
> serious enough to cause a segfault.

Wow.

> What happened?
> 
> My first mistake was in jsrt.
> I didn't include any tests for the URL class.
> That class is very special and very complicated, over a thousand lines of code.
> With no regression tests, bugs could creep in and nobody would know.
> And they did.

Ah.  Are there any other missing tests from jsrt while we're here?

> My latest push adds tests for this class, so this won't happen again.
> See jsrt line 536.

Good to hear, when I've not got large amounts of university work to do I'll have a look.

> I'm not going to describe all the bugs, but here is a funny one.
> We had, for a long time, code like this:
> 
> if(condition)
> do this
> else
> do that
> 
> But no error legs in js was leading to nulls and segfaults,
> so we put in error legs all over the place.
> This we needed to do, and thanks to everyone for your help.
> There were so many, the process was rather mechanical,
> and understandably so.
> Just get er done.
> Thus the above code became:
> 
> if(condition)
> if(do this and it fails) {
> js fail
> return
> }
> else
> if(do that and it fails) {
> js fail
> return
> }
> 
> If you've studied formal language theory and compilers,
> then you know this is the region of the C grammar that is ambiguous.
> But the compiler doesn't tell you that,
> it just picks a derivation and marches on.

Yep.

> Which one?
> Not the one we expect.

Yeah, it certainly isn't imediately obvious from reading the code what happens 
here,and I suspect I missed these when reviewing... sorry.

> I changed the code as below and several bugs went away.
> 
> if(condition) {
> if(do this and it fails) {
> js fail
> return
> }
> } else {
> if(do that and it fails) {
> js fail
> return
> }
> }
> 
> There were quite a few blocks like this.

Yeah I'm not surprised and well done for figuring out what the logic *should* 
have been.

> So there you go - bitten by the if else ambiguity of C.

Yep and by the sounds of it quite badly.
I'm now wondering if there are any other bugs of this type waiting to be 
discovered.

> There were other bugs too, dealing with string allocation
> and a malformed url constructor.
> I think they are fixed now; jsrt passes anyways.
> I'm glad I cleaned all this up before release 3.5.1.

So am I, again well done for fixing these.

Cheers,
Adam.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]