IPB

Welcome Guest ( Log In | Register )

 
Reply to this topicStart new topic
Old WavPack, weird code sequences
kode54
post Sep 30 2013, 01:06
Post #1





Group: Admin
Posts: 4504
Joined: 15-December 02
Member No.: 4082



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
                    sum = left + (right = (left = next_word) - diff);


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

CODE
                    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.
Go to the top of the page
+Quote Post
nu774
post Sep 30 2013, 04:25
Post #2





Group: Developer
Posts: 477
Joined: 22-November 10
From: Japan
Member No.: 85902



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).
Go to the top of the page
+Quote Post
db1989
post Sep 30 2013, 07:43
Post #3





Group: Super Moderator
Posts: 5173
Joined: 23-June 06
Member No.: 32180



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
sum = left + (right = (left = next_word) - diff);
becoming
CODE
left = next_word;
right = left - diff;
sum = left + right;
and
CODE
sum = (left = diff + (right = next_word)) + right;
becoming
CODE
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.
Go to the top of the page
+Quote Post
bryant
post Sep 30 2013, 08:26
Post #4


WavPack Developer


Group: Developer (Donating)
Posts: 1287
Joined: 3-January 02
From: San Francisco CA
Member No.: 900



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.
Go to the top of the page
+Quote Post
nu774
post Sep 30 2013, 09:01
Post #5





Group: Developer
Posts: 477
Joined: 22-November 10
From: Japan
Member No.: 85902



It seems unpack3.c of 4.60.1 is OK. In what version that code is in?

CODE
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)
Go to the top of the page
+Quote Post
nu774
post Sep 30 2013, 09:21
Post #6





Group: Developer
Posts: 477
Joined: 22-November 10
From: Japan
Member No.: 85902



Simpler example:
CODE
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.
Go to the top of the page
+Quote Post
db1989
post Sep 30 2013, 19:09
Post #7





Group: Super Moderator
Posts: 5173
Joined: 23-June 06
Member No.: 32180



QUOTE (bryant @ Sep 30 2013, 08:26) *
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. wink.gif 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. biggrin.gif
Go to the top of the page
+Quote Post
kode54
post Sep 30 2013, 19:20
Post #8





Group: Admin
Posts: 4504
Joined: 15-December 02
Member No.: 4082



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.)
Go to the top of the page
+Quote Post
bryant
post Sep 30 2013, 19:20
Post #9


WavPack Developer


Group: Developer (Donating)
Posts: 1287
Joined: 3-January 02
From: San Francisco CA
Member No.: 900



QUOTE (nu774 @ Sep 30 2013, 01:01) *
It seems unpack3.c of 4.60.1 is OK. In what version that code is in?

CODE
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... sad.gif

CODE
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.
Go to the top of the page
+Quote Post
bryant
post Sep 30 2013, 19:59
Post #10


WavPack Developer


Group: Developer (Donating)
Posts: 1287
Joined: 3-January 02
From: San Francisco CA
Member No.: 900



QUOTE (db1989 @ Sep 30 2013, 11:09) *
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. biggrin.gif
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.
Go to the top of the page
+Quote Post
db1989
post Sep 30 2013, 20:01
Post #11





Group: Super Moderator
Posts: 5173
Joined: 23-June 06
Member No.: 32180



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 tongue.gif

Interesting info again. smile.gif I might check out that book.
Go to the top of the page
+Quote Post
bryant
post Sep 30 2013, 20:22
Post #12


WavPack Developer


Group: Developer (Donating)
Posts: 1287
Joined: 3-January 02
From: San Francisco CA
Member No.: 900



QUOTE (db1989 @ Sep 30 2013, 12:01) *
Interesting info again. smile.gif 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.
Go to the top of the page
+Quote Post
bryant
post Sep 30 2013, 20:44
Post #13


WavPack Developer


Group: Developer (Donating)
Posts: 1287
Joined: 3-January 02
From: San Francisco CA
Member No.: 900



QUOTE (db1989 @ Sep 30 2013, 12:01) *
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 tongue.gif

If you happen to write C code for a living, it's a great way to annoy your co-workers! smile.gif
Go to the top of the page
+Quote Post
kode54
post Sep 30 2013, 21:53
Post #14





Group: Admin
Posts: 4504
Joined: 15-December 02
Member No.: 4082



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.)
Go to the top of the page
+Quote Post
bryant
post Oct 2 2013, 01:03
Post #15


WavPack Developer


Group: Developer (Donating)
Posts: 1287
Joined: 3-January 02
From: San Francisco CA
Member No.: 900



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?
Go to the top of the page
+Quote Post
kode54
post Oct 2 2013, 12:33
Post #16





Group: Admin
Posts: 4504
Joined: 15-December 02
Member No.: 4082



Assuming I can't get the original developer to accept a pull request of random changes all hammered out into a single master branch.
Go to the top of the page
+Quote Post
kode54
post Oct 14 2013, 09:15
Post #17





Group: Admin
Posts: 4504
Joined: 15-December 02
Member No.: 4082



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.
Go to the top of the page
+Quote Post
bryant
post Oct 15 2013, 20:08
Post #18


WavPack Developer


Group: Developer (Donating)
Posts: 1287
Joined: 3-January 02
From: San Francisco CA
Member No.: 900



QUOTE (kode54 @ Oct 14 2013, 01:15) *
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.
Go to the top of the page
+Quote Post

Reply to this topicStart new topic
1 User(s) are reading this topic (1 Guests and 0 Anonymous Users)
0 Members:

 



RSS Lo-Fi Version Time is now: 23rd April 2014 - 19:32