Re: I won't bother to rewrite all of the code, so only for ParseColorTuple:

Posted Dec 26, 2014 2:12 UTC (Fri) by ldo (guest, #40946)
In reply to: Re: Experience shows that idiomatic "goto error_exit" is far better than the alternatives with fake loops and flags. by Cyberax
Parent article: The "too small to fail" memory-allocation rule

At least you made the effort, even if you “fixed” a non-bug and succeeded in introducing a real bug.

This was a simple, almost trivial case. Now try a more complex one. Elsewhere you will see do-once constructs nested up to 3 deep. And then there’s loops. Try and see how well your goto constructs scale up to those complexities.

to post comments

Re: I won't bother to rewrite all of the code, so only for ParseColorTuple:

Posted Dec 26, 2014 4:59 UTC (Fri) by Cyberax (✭ supporter ✭, #52523) [Link] (22 responses)

> and succeeded in introducing a real bug.
Where?

> This was a simple, almost trivial case. Now try a more complex one.
You can do it _automatically_ by rewriting do-nothing loops into 'goto cleanup' and merging cleanup actions if needed.

> Elsewhere you will see do-once constructs nested up to 3 deep.
That's not something to be proud of, you know. It means that you've failed to decompose your logic into smaller fragments.

And the rest of your code...

It pretty much falls into the 'unmaintainable crap' bin. Everything is just perfect: eclectic code style, exotic constructs (inner functions in... C?), profligate usage of goto disguised as do..nothing loops, functions spanning hundreds of lines, etc.

Re: It pretty much falls into the 'unmaintainable crap' bin

Posted Dec 26, 2014 5:34 UTC (Fri) by ldo (guest, #40946) [Link] (20 responses)

Too bad, that your programming ideas only work with toy examples, not with real-world code.

Come back when you’ve learned a bit more about programming.

Re: It pretty much falls into the 'unmaintainable crap' bin

Posted Dec 26, 2014 5:40 UTC (Fri) by Cyberax (✭ supporter ✭, #52523) [Link] (13 responses)

> Too bad, that your programming ideas only work with toy examples, not with _real-world code_.
Like... I don't know, say an OS kernel?

Or perhaps an audio codec? Nah, can't write those with gotos: https://git.xiph.org/?p=opus.git;a=blob;f=src/opus_encoder.c;...

Re: Or perhaps an audio codec?

Posted Dec 26, 2014 21:21 UTC (Fri) by ldo (guest, #40946) [Link] (12 responses)

While I’m sure it is quite complex code, it doesn’t seem to do much with dynamic storage: I found just two calls to opus_free in those 2500-odd lines.

While my example is half that size, it has 39 calls to PY_{,X}DECREF.

Come back when you can cope with that kind of complexity.

Re: Or perhaps an audio codec?

Posted Dec 26, 2014 21:25 UTC (Fri) by Cyberax (✭ supporter ✭, #52523) [Link] (11 responses)

Your wish is my command: http://lxr.free-electrons.com/source/drivers/net/tun.c#L1741

Re: Your wish is my command

Posted Dec 27, 2014 20:56 UTC (Sat) by ldo (guest, #40946) [Link] (10 responses)

Now try one with a loop.

Re: Your wish is my command

Posted Dec 27, 2014 20:58 UTC (Sat) by Cyberax (✭ supporter ✭, #52523) [Link] (9 responses)

What do you mean by "with a loop"? And feel free to try it yourself, since you're such a religious fanatic of do-nothing loops.

Re: What do you mean by "with a loop"?

Posted Dec 27, 2014 21:43 UTC (Sat) by ldo (guest, #40946) [Link] (8 responses)

My code has examples of loops where errors can occur in them. So I need to deal gracefully with that. Let’s see you offer an example that does the same.

And while we’re at it, your tun.c example fails to recover if it cannot create its sysfs device files.

Re: What do you mean by "with a loop"?

Posted Dec 27, 2014 22:01 UTC (Sat) by Cyberax (✭ supporter ✭, #52523) [Link] (7 responses)

> My code has examples of loops where errors can occur in them. So I need to deal gracefully with that.
No you don't. Just split them up into free-standing functions, instead of 300-line monstrosities.

The Linux kernel manages to do that just fine, somehow.

In particular, your block near line 480 will look like this:
https://gist.github.com/Cyberax/3a7796231be66d0f64cc
(no, I'm not going to check it in details). It's pretty clear that simply by splitting some logic into a function you can decrease nesting level by 3.

And let me reiterate, your code is a mess. For example, you use 'break' to normally exit the outer loop from within the 'if' statement here: https://github.com/ldo/dvd_menu_animator/blob/master/spuh...

Why do you even bother with 'for' loops?

Re: And let me reiterate, your code is a mess.

Posted Dec 28, 2014 6:29 UTC (Sun) by ldo (guest, #40946) [Link] (6 responses)

At least it works, unlike your code.

Your for-loop will never iterate.
Where is PixBuf supposed to be local to?

Pot calling the kettle black, as it were?

Re: And let me reiterate, your code is a mess.

Posted Dec 28, 2014 6:31 UTC (Sun) by Cyberax (✭ supporter ✭, #52523) [Link] (5 responses)

I have not claimed that my code works (as I wrote it on a dumb web form without even automatic indentation). But it's far easier to understand and fix than your mess of twisty little loops, all alike.

Re: I have not claimed that my code works

Posted Dec 28, 2014 6:36 UTC (Sun) by ldo (guest, #40946) [Link] (4 responses)

Then what is the point? I thought you were trying to demonstrate that you could do the same thing more simply and/or clearly than I could. But instead you have totally stuffed it up.

Re: I have not claimed that my code works

Posted Dec 28, 2014 6:41 UTC (Sun) by Cyberax (✭ supporter ✭, #52523) [Link] (3 responses)

I don't really care to try to understand what your code actually means. It's so convoluted that it's absolute unmaintainable.

If you can make a reasonable description of what it does - I can guarantee that it's possible to write it far cleaner then what you have.

Re: If you can make a reasonable description of what it does

Posted Dec 28, 2014 21:33 UTC (Sun) by ldo (guest, #40946) [Link] (2 responses)

Did you not read the comments? There are even Python docstrings; did you not see those?

Re: If you can make a reasonable description of what it does

Posted Dec 28, 2014 22:37 UTC (Sun) by Cyberax (✭ supporter ✭, #52523) [Link] (1 responses)

No, they don't describe the algorithm. Please, provide a coherent description in a natural language (I understand English, Russian, Ukrainian, German and Polish) then I can provide you its implementation that will be more compact and easy-to-understand than yours.

Re: Please, provide a coherent description

Posted Dec 29, 2014 17:26 UTC (Mon) by ldo (guest, #40946) [Link]

Funny, I didn’t see any such thing in the code examples you saw fit to refer me to. Yet you expected me to cope with them. And my code already has far more comments than they did.

What’s good for the goose, eh?

Re: It pretty much falls into the 'unmaintainable crap' bin

Posted Dec 26, 2014 16:14 UTC (Fri) by mb (subscriber, #50428) [Link] (5 responses)

>Too bad, that your programming ideas only work with toy examples, not with real-world code.

This is like the guy complaining about _all_ those damn ghost drivers on the highway.

Huge projects use goto for error handling successfully. See the Linux kernel for instance.
And your "nested pseudo loops error handler" argument is void, because thouse unmaintainable monsters should be split into sub-routines anyway. And then it can use plain gotos. Which gains the additional opportunity to write structured error paths instead of "one must handle all errors" error paths.

Re: Huge projects use goto for error handling successfully

Posted Dec 26, 2014 21:15 UTC (Fri) by ldo (guest, #40946) [Link] (4 responses)

Or perhaps not so successfully. Otherwise we would not have this article.

Re: Huge projects use goto for error handling successfully

Posted Dec 27, 2014 11:32 UTC (Sat) by mb (subscriber, #50428) [Link] (3 responses)

The article talks (as a side note) about untested error paths.
Your complex pseudo-loop can't help here at all. It just adds extra complexity. The error paths will still be untested.

Re: The error paths will still be untested.

Posted Dec 27, 2014 20:40 UTC (Sat) by ldo (guest, #40946) [Link] (2 responses)

It makes them easier to see. That helps in writing the code in the first place.

Re: The error paths will still be untested.

Posted Dec 27, 2014 23:14 UTC (Sat) by vonbrand (subscriber, #4458) [Link] (1 responses)

The pseudo-loops hide the error processing in a structure that could be anything else, and which BTW I haven't seen anywhere else either. ln comparison, the "label: <error handling code>" pattern is easy to spot.

Re: The pseudo-loops hide the error processing

Posted Dec 28, 2014 6:33 UTC (Sun) by ldo (guest, #40946) [Link]

No, the cleanups always happen after the do-once constructs. That’s how you can be sure they will always be executed.

Re: I won't bother to rewrite all of the code, so only for ParseColorTuple:

Posted Dec 26, 2014 16:30 UTC (Fri) by nix (subscriber, #2304) [Link]

(do/once constructs nested up to three deep)

That's not something to be proud of, you know. It means that you've failed to decompose your logic into smaller fragments.

Actually it means he's probably trying to do multiple things which need unwinding, but because he can't use three goto labels and rely on fallthrough he has to add a layer of almost-entirely-redundant nesting for each one.

This is not something that you'd call out as an advantage and a sign of great experience unless you didn't actually have much. (But it's been worth reading this thread anyway: it's not every day you see people telling Al Viro that he doesn't have enough experience to judge the relative worth of exception-handling goto versus other techniques!)