Skip to main content

Topic: Old WavPack, weird code sequences (Read 8514 times) previous topic - next topic

0 Members and 1 Guest are viewing this topic.
  • kode54
  • [*][*][*][*][*]
  • Administrator
Old WavPack, weird code sequences
Before I immediately fetch and integrate a newer version of WavPack into this project, I would like to know what to make of these two assignment sequences from unpack3.c:

Code: [Select]
                    sum = left + (right = (left = next_word) - diff);


Is the left assignment supposed to occur before it is accessed on the left?

Code: [Select]
                    sum = (left = diff + (right = next_word)) + right;


What about right here? Accessed on the right before or after the assignment inside?

These seem just a bit weird.

  • nu774
  • [*][*][*][*][*]
  • Developer
Old WavPack, weird code sequences
Reply #1
Looks like undefined behavior to me. gcc will point it out if -Wall (more specifically, -Wsequence-point) is set.
In my environment, value of "sum" is different between gcc and clang (and that may be dependent on compiler options or something).

  • db1989
  • [*][*][*][*][*]
  • Global Moderator
Old WavPack, weird code sequences
Reply #2
Ugh, this is why allowing assignment in any context other than an isolated variable = value; statement is a horrible idea and should never have been allowed out of the first draft of C.

Kinda surprised this is undefined behaviour, though. As far as I can see from reading various tables, the = used for assignment is placed way down in the order of evaluation/precedence of operators, just above the comma (another silly idea that should have been left out), and parallel assignments are processed from right to left. with the aid of brackets forcing things forward, we should get:
Code: [Select]
sum = left + (right = (left = next_word) - diff);
becoming
Code: [Select]
left = next_word;
right = left - diff;
sum = left + right;
and
Code: [Select]
sum = (left = diff + (right = next_word)) + right;
becoming
Code: [Select]
right = next_word;
left = diff + right;
sum = left + right;

Either way, the original compound statements look horrible and obfuscate understanding. If some compilers disagree on how to interpret them, whether or not there is any theoretical ambiguity, I can happily take another excuse for everyone to avoid using embedded assignments!

Hands up: I kinda see the appeal of creating complicated, maximally crunched statements in a geeky way, sort of like a vastly reduced personal version of the IOCCC. But for any real project, especially if it were likely to be released, I would have to avoid them, and this is as much for my own ability to understand the code the next day as anything else! Creating technically baffling code can be a fun exercise, but it tends to mess things up down the line.

  • bryant
  • [*][*][*][*][*]
  • Developer (Donating)
Old WavPack, weird code sequences
Reply #3
For these examples (and any others that you find in the WavPack code) you can assume that the parenthesis specify the evaluation order (deeper expressions are executed first). I know that this is not actually correct and I will fix them before the next release; thanks for pointing them out to me.

The weird thing is that I haven't had a compiler complain about these before (either gcc or MSVC) even in the most sensitive warning modes, and I just tried -Wsequence-point with gcc 4.7.2 and got nothing.

To answer db1989's points, these were done for performance reasons. I know that [some] people will always claim that compilers are far better optimizers than human programmers, but I know from firsthand experience that statements like these are often faster. Without looking at the assembler, I would guess that both statements eliminate two loads from the code. You would think that the compiler could easily keep intermediate values in registers between statements, but, as least when I wrote this 10+ years ago, they didn't.

And yes, I do enjoy writing compact code, and will not usually write code with extra steps and intermediate variables just to make it clearer (especially if I want something to going into ffmpeg!), but these were obviously wrong.

  • nu774
  • [*][*][*][*][*]
  • Developer
Old WavPack, weird code sequences
Reply #4
It seems unpack3.c of 4.60.1 is OK. In what version that code is in?

Code: [Select]
sum = left + (right = (left = next_word) - diff);

This will trigger gcc warning, but I couldn't find exactly this one in unpack3.c.
The wording of following specification is somewhat unclear to me, but this code seems to conflict with the second sentence.

Quote
Between the previous and next sequence point an object shall have its stored value modified at most once by the evaluation of an expression.72) Furthermore, the prior value shall be read only to determine the value to be stored.73)

  • nu774
  • [*][*][*][*][*]
  • Developer
Old WavPack, weird code sequences
Reply #5
Simpler example:
Code: [Select]
x = x + 1; // OK
y = x = 1; // OK
y = x + (x = 1); // undefined behavior, warned by -Wsequence-point

Usage of variable "left" in that example is about the same as the last one.

  • db1989
  • [*][*][*][*][*]
  • Global Moderator
Old WavPack, weird code sequences
Reply #6
To answer db1989's points, these were done for performance reasons. I know that [some] people will always claim that compilers are far better optimizers than human programmers, but I know from firsthand experience that statements like these are often faster. Without looking at the assembler, I would guess that both statements eliminate two loads from the code. You would think that the compiler could easily keep intermediate values in registers between statements, but, as least when I wrote this 10+ years ago, they didn't.
This is very interesting. Thanks for explaining. I never meant to imply this was bad programming at all, more a regrettable feature of the language, which is IMHO as a relative layman to programming. That aside, now that these features exist, I have nothing against people employing them if it turns out there is a valid reason to do so. And you have a very valid one! Or at least had ten years ago.  I wonder whether things have evolved past this since then.

Quote
And yes, I do enjoy writing compact code, and will not usually write code with extra steps and intermediate variables just to make it clearer (especially if I want something to going into ffmpeg!), but these were obviously wrong.
True. Maybe I exaggerated earlier because I used to love writing cryptic code, but I ended up kicking myself whenever I went back to read it not that much later.

  • kode54
  • [*][*][*][*][*]
  • Administrator
Old WavPack, weird code sequences
Reply #7
Holy crap, it's 4.32. I'll see about upgrading as soon as possible.

Also, it was detected by:

Apple LLVM version 5.0 (clang-500.2.75) (based on LLVM 3.3svn)
Target: x86_64-apple-darwin12.5.0

(For anyone who cares, it's the version that's bundled with Cog, which I've forked here.)
  • Last Edit: 30 September, 2013, 02:25:51 PM by kode54

  • bryant
  • [*][*][*][*][*]
  • Developer (Donating)
Old WavPack, weird code sequences
Reply #8
It seems unpack3.c of 4.60.1 is OK. In what version that code is in?

Code: [Select]
sum = left + (right = (left = next_word) - diff);

This will trigger gcc warning, but I couldn't find exactly this one in unpack3.c.

You're right! These errors were actually flagged by -Wall and fixed in 2006. I guess my memory is starting to go... 

Code: [Select]
r10 | dbryant | 2006-08-25 17:55:57 -0700 (Fri, 25 Aug 2006) | 2 lines

eliminate most gcc -Wall warnings

Yes, kode54, it probably wouldn't be a bad idea to grab a fresh repo, although you might want to wait until 4.70 is released.

  • bryant
  • [*][*][*][*][*]
  • Developer (Donating)
Old WavPack, weird code sequences
Reply #9
Quote
And yes, I do enjoy writing compact code, and will not usually write code with extra steps and intermediate variables just to make it clearer (especially if I want something to going into ffmpeg!), but these were obviously wrong.
True. Maybe I exaggerated earlier because I used to love writing cryptic code, but I ended up kicking myself whenever I went back to read it not that much later.
For better or worse, we are all products of our learning. I learned C from the first [pre-ANSI] Kernighan & Ritchie book and it's obvious that they added many of these “shortcut” features to C because they enjoyed writing concise code (and their compiler was probably not optimizing yet), and I certainly picked up the habit. I still remember their example of presenting a series of strcpy() implementions until they had gotten it down to a single “while” statement with no body (all the code was in the condition). And then they said:

Quote
Although this may seem cryptic at first sight, the notational convenience is considerable, and the idiom should be mastered, because you will see it frequently in C programs.

  • db1989
  • [*][*][*][*][*]
  • Global Moderator
Old WavPack, weird code sequences
Reply #10
I bet now I will soon find myself unable to resist the temptation to see how far I can reduce some of my own code! At least I have someone to blame

Interesting info again.  I might check out that book.

  • bryant
  • [*][*][*][*][*]
  • Developer (Donating)
Old WavPack, weird code sequences
Reply #11
Interesting info again.  I might check out that book.

A free pdf copy of the ANSI version has been floating around for years (like here) and I would recommend that over the pre-ANSI version I used. Pre-ANSI C just looks weird now.

  • bryant
  • [*][*][*][*][*]
  • Developer (Donating)
Old WavPack, weird code sequences
Reply #12
I bet now I will soon find myself unable to resist the temptation to see how far I can reduce some of my own code! At least I have someone to blame

If you happen to write C code for a living, it's a great way to annoy your co-workers! 

  • kode54
  • [*][*][*][*][*]
  • Administrator
Old WavPack, weird code sequences
Reply #13
All updated to 4.60.1. I also had to fix a piece of code which was scaling floating point samples by 32767 instead of INT_MAX. (Since this thing uses all integer formats for playback.)

  • bryant
  • [*][*][*][*][*]
  • Developer (Donating)
Old WavPack, weird code sequences
Reply #14
That’s great you’re keeping Cog updated (and thanks for getting the newer WavPack decoder in there)!

It’s one of the players that I have linked to from my site for a long time; I suppose at some point I should change my link to your version?

  • kode54
  • [*][*][*][*][*]
  • Administrator
Old WavPack, weird code sequences
Reply #15
Assuming I can't get the original developer to accept a pull request of random changes all hammered out into a single master branch.

  • kode54
  • [*][*][*][*][*]
  • Administrator
Old WavPack, weird code sequences
Reply #16
Bah, I totally missed that https://bitbucket.org/mamburu/cog forked it already, and maintains a version that cogx.org's stable.php redirects to. I spent about a day or two importing miscellaneous fixes from that.

  • bryant
  • [*][*][*][*][*]
  • Developer (Donating)
Old WavPack, weird code sequences
Reply #17
Bah, I totally missed that https://bitbucket.org/mamburu/cog forked it already, and maintains a version that cogx.org's stable.php redirects to. I spent about a day or two importing miscellaneous fixes from that.

I took a quick look at that fork and see they still have the old WavPack library code. Hopefully a merge will be possible at some point.