Re: Please don't do this (code fragment)
[EMAIL PROTECTED] writes: I don't find any such mention in KR [1978]. There is nothing I can see to guarantees that MAX_INT + 1 0. KR [1978] is not the C standard.
Re: Please don't do this (code fragment)
Good grief. Can you be more constructive? Do you have a reference that supports the claim? On Tue, Jan 15, 2002 at 12:11:35AM -0800, Thomas Bushnell, BSG wrote: [EMAIL PROTECTED] writes: I don't find any such mention in KR [1978]. There is nothing I can see to guarantees that MAX_INT + 1 0. KR [1978] is not the C standard.
Re: Please don't do this (code fragment)
Thomas Bushnell writes: Actually, the C standard does essentially guarantee two's complement arithmetic. It specifies integer overflow behavior and signed/unsigned conversion behavior exactly. It does for unsigned integers, but for signed integers overflow is undefined behaviour. The clearest statement of that is 3.4.3, albeit in an example: 3.4.3 1 undefined behavior behavior, upon use of a nonportable or erroneous program construct or of erroneous data, for which this International Standard imposes no requirements 2 NOTE Possible undefined behavior ranges from ignoring the situation completely with unpredictable results, to behaving during translation or program execution in a documented manner characteristic of the environment (with or without the issuance of a diagnostic message), to terminating a translation or execution (with the issuance of a diagnostic message). 3 EXAMPLE An example of undefined behavior is the behavior on integer overflow. -- Olaf Weber (This space left blank for technical reasons.)
Re: Please don't do this (code fragment)
On 13/01, [EMAIL PROTECTED] wrote:
| int i;
| for (i = 0; i -1; i += 1) {
| // ...
| if (terminal_condition)
| break;
| // ...
| }
[...]
| Moreover, i is never used. The loop could be reduced to
|
| while ((file = fts_read (dir)) != NULL) {
| // ...
| }
Those are not equivalent: the first loop, while ugly, has a guard against
endless looping if fts_read always returns non-NULL for any reason (not
knowing what fts_read contains, it is hard to tell whether there is a
reason for this or not).
Sam
Re: Please don't do this (code fragment)
On Sunday 13 January 2002 10:01 pm, Samuel Tardieu wrote:
On 13/01, [EMAIL PROTECTED] wrote:
| int i;
| for (i = 0; i -1; i += 1) {
| // ...
| if (terminal_condition)
| break;
| // ...
| }
[...]
| Moreover, i is never used. The loop could be reduced to
|
| while ((file = fts_read (dir)) != NULL) {
| // ...
| }
Those are not equivalent: the first loop, while ugly, has a guard against
endless looping if fts_read always returns non-NULL for any reason (not
knowing what fts_read contains, it is hard to tell whether there is a
reason for this or not).
but it can still be reduced to a single while loop while retaining the same
behavior as the original code:
while(file = fts_read(dir)) {
// ...
}
Re: Please don't do this (code fragment)
On Mon, Jan 14, 2002 at 07:01:17AM +0100, Samuel Tardieu wrote:
On 13/01, [EMAIL PROTECTED] wrote:
| int i;
| for (i = 0; i -1; i += 1) {
| // ...
| if (terminal_condition)
| break;
| // ...
| }
[...]
| Moreover, i is never used. The loop could be reduced to
|
| while ((file = fts_read (dir)) != NULL) {
| // ...
| }
Those are not equivalent: the first loop, while ugly, has a guard against
endless looping if fts_read always returns non-NULL for any reason (not
knowing what fts_read contains, it is hard to tell whether there is a
reason for this or not).
A poor reason to write that code for several reasons.
1) It bounds the loop to the size of the integer /2 without
explicitly indicating so.
2) If there is a problem, it silently hide the fact.
3) It is non-deterministic. Should the loop fail to terminate
properly, the program will run for a relatively long time. 15
seconds on the PIII 700MHz I'm using.
While I don't agree that this is an appropriate method for
guaranteeing termination, the same result with clearer expression
would be
int loop_max = MAX_INT;
while ((file = fts_read (dir)) != NULL --loop_max) {
}
I'd rather see the program fail to terminate so that I knew something
was awry instead of imposing an unforseen limitation on the program.
Moreover, the original depends on the fact that (MAX_INT + 1) 0
which, though true, is a guaranteed result. Were I to be running a 64
bit CPU, the time to terminate either of these loops would approach
infinity (2^32*15s)-~2042 years. I don't see the point of using such
a weak attempt to guarantee termination.
Cheers.
Re: Please don't do this (code fragment)
[EMAIL PROTECTED] wrote:
On Mon, Jan 14, 2002 at 07:01:17AM +0100, Samuel Tardieu wrote:
On 13/01, [EMAIL PROTECTED] wrote:
| int i;
| for (i = 0; i -1; i += 1) {
| // ...
| if (terminal_condition)
| break;
| // ...
| }
[...]
| Moreover, i is never used. The loop could be reduced to
|
| while ((file = fts_read (dir)) != NULL) {
| // ...
| }
Those are not equivalent: the first loop, while ugly, has a guard against
endless looping if fts_read always returns non-NULL for any reason (not
knowing what fts_read contains, it is hard to tell whether there is a
reason for this or not).
A poor reason to write that code for several reasons.
Samuel wasn't defending the original code; he was just observing that
your suggested replacement was not functionally equivalent. Which it
wasn't.
1) It bounds the loop to the size of the integer /2 without
explicitly indicating so.
It's pretty clear if you understand how twos-complement arithmetic
works. Which is not to say it's the right thing to do, but I've seen
code that is much harder than this to understand.
2) If there is a problem, it silently hide the fact.
It's impossible to know that based on the code fragment you posted.
There's no indication of what happens after the loop, or even most of
the inside of the loop. I'm sure you're familiar with the rest of the
code, but the rest of us probably aren't.
3) It is non-deterministic. Should the loop fail to terminate
properly, the program will run for a relatively long time. 15
seconds on the PIII 700MHz I'm using.
The original code is deterministic (the loop will definitely exit sooner
or later); your suggested replacement is not, unless it is guaranteed
that fts_read will eventually return NULL.
While I don't agree that this is an appropriate method for
guaranteeing termination, the same result with clearer expression
would be
int loop_max = MAX_INT;
while ((file = fts_read (dir)) != NULL --loop_max) {
}
That's clearer, yes. It still sucks, unless there's a really good reason
to use MAX_INT as the limit. It's hard to say without knowing more about
the context of this tiny fragment of code, but I would guess that since
the loop condition is only used to guarantee eventual termination, the
limit could reasonably be set to a constant that is consistent across
platforms (whatever that constant might be -- 10^3, 10^6, 10^9,
whatever), rather than one that will vary by multiple orders of
magnitude depending on whether the target system uses 16-, 32-, or
64-bit ints.
I'd rather see the program fail to terminate so that I knew something
was awry instead of imposing an unforseen limitation on the program.
I'd rather see the program send a useful diagnostic message to stderr or
a log file, and either exit or properly recover from the error.
Craig
pgprIVUWj8rck.pgp
Description: PGP signature
Re: Please don't do this (code fragment)
Reply-To:
In-Reply-To: [EMAIL PROTECTED]
On Sun, Jan 13, 2002 at 10:49:09PM -0800, Craig Dickson wrote:
[EMAIL PROTECTED] wrote:
On Mon, Jan 14, 2002 at 07:01:17AM +0100, Samuel Tardieu wrote:
On 13/01, [EMAIL PROTECTED] wrote:
| int i;
| for (i = 0; i -1; i += 1) {
| // ...
| if (terminal_condition)
| break;
| // ...
| }
[...]
| Moreover, i is never used. The loop could be reduced to
|
| while ((file = fts_read (dir)) != NULL) {
| // ...
| }
Those are not equivalent: the first loop, while ugly, has a guard against
endless looping if fts_read always returns non-NULL for any reason (not
knowing what fts_read contains, it is hard to tell whether there is a
reason for this or not).
A poor reason to write that code for several reasons.
Samuel wasn't defending the original code; he was just observing that
your suggested replacement was not functionally equivalent. Which it
wasn't.
That is the crux of my argument. In a 64 bit environment, this code
as written *is* equivalent to my rewrite.
1) It bounds the loop to the size of the integer /2 without
explicitly indicating so.
It's pretty clear if you understand how twos-complement arithmetic
works. Which is not to say it's the right thing to do,
Of course that is true. Yet, such practices define poor style. The
original author isn't writing what he means. The fact that 2's
complement arithmetic wraps is a side-effect. At least the use of
MAX_INT is an honest expression of the loop intent.
but I've seen code that is much harder than this to understand.
And your point is what? I'm saying nothing about the difficulty of
reading the code. There is little point in having that discussion.
3) It is non-deterministic. Should the loop fail to terminate
properly, the program will run for a relatively long time. 15
seconds on the PIII 700MHz I'm using.
The original code is deterministic (the loop will definitely exit sooner
or later); your suggested replacement is not, unless it is guaranteed
that fts_read will eventually return NULL.
This is the kind of code that tends to break as systems evolve. When
we moved from 16 bit to 32 bit, dependencies on machine organization
broke many programs that depended on them. As I wrote in my reply, a
64 bit machine would have the same observable behavior as one without
the weak loop-termination guarantee. Anything that requires more than
a month to terminate when one expects it to terminate in a few of
minutes has effectively hung.
While I don't agree that this is an appropriate method for
guaranteeing termination, the same result with clearer expression
would be
int loop_max = MAX_INT;
while ((file = fts_read (dir)) != NULL --loop_max) {
}
That's clearer, yes. It still sucks, unless there's a really good reason
to use MAX_INT as the limit. It's hard to say without knowing more about
the context of this tiny fragment of code, but I would guess that since
the loop condition is only used to guarantee eventual termination, the
limit could reasonably be set to a constant that is consistent across
platforms (whatever that constant might be -- 10^3, 10^6, 10^9,
whatever), rather than one that will vary by multiple orders of
magnitude depending on whether the target system uses 16-, 32-, or
64-bit ints.
Still a poor choice. You are hiding a limitation in the code that is
neither appropriate nor desirable. Limiting the number of files, no
matter how you do it, will always change the semantics of the program.
Can you imaging people's suprise when the generate a search database
that appears to be missing a file that they know exists? Setting a
time limit on the program run may be a more reliable standard; but
this, too, will be difficult to bound. A process that runs without
bound will, at least, make itself known by loading the system.
I'd rather see the program fail to terminate so that I knew something
was awry instead of imposing an unforseen limitation on the program.
I'd rather see the program send a useful diagnostic message to stderr or
a log file, and either exit or properly recover from the error.
Of couse. I agree that the code would be better if there was a
reasonable bound on the extent of the loop *AND* it reports that
something went awry. But what should be the criteria?
The point is that the original code fragment fails to perform the
desired task deterministicly. Within a few years, there will be a
significant number of machines running 64 bit CPUs. Programs that use
this poor expression will be no better than those that don't. I
believe this is the sort of failure-protection that Linus Torvalds
complains about. Silently tolerating an error is worse than
discovering it when it executes without bound.
Cheers.
Re: Please don't do this (code fragment)
On Sun, Jan 13, 2002 at 10:23:51PM -0800, [EMAIL PROTECTED] wrote: Moreover, the original depends on the fact that (MAX_INT + 1) 0 which, though true, is a guaranteed result. Were I to be running a 64 ... is not a guaranteed ... I yet have to see a machine that does not use two's complement for integer arithmetic. cu Torsten pgpNT7VyvELTp.pgp Description: PGP signature
Re: Please don't do this (code fragment)
On Mon, Jan 14, 2002 at 12:04:44AM -0800, [EMAIL PROTECTED] wrote: [How to write bad code] Why don't you man fts_read and implement the correct way of checking for errors? -- Met vriendelijke groet / with kind regards, Guus Sliepen [EMAIL PROTECTED] pgpuwz8FVAQoR.pgp Description: PGP signature
Re: Please don't do this (code fragment)
And what's more, it only indents two at a time. Heretics! -- David N. Welton Consulting: http://www.dedasys.com/ Free Software: http://people.debian.org/~davidw/ Apache Tcl: http://tcl.apache.org/ Personal: http://www.efn.org/~davidw/
Re: Please don't do this (code fragment)
Richard Kettlewell wrote: Even if you don't care about weird platforms, x -1 is a ridiculously obscure test in this context; to achieve the same effect it would be much clearer to make x unsigned and do x = (unsigned)INT_MAX. I find x = (unsigned) INT_MAX to be more obscure than the original. When I first glanced at it, I thought, That's dumb, x is ALWAYS less than or equal to INT_MAX by definition! Then my second thought was that the cast on the constant will cause x to also be converted to unsigned. In contrast, when I saw x -1, I immediately realized it was testing for wraparound. To achieve almost the same effect (probably close enough in this situation), it would be better to simply test for x INT_MAX -- it's clearer than either the original or your cast-dependent version, and only stops one iteration sooner. Since in this case the actual number of iterations was not the point (rather, merely that the loop should be guaranteed to terminate eventually), it ought to be sufficient. In any case, trying to find the best way to express a really bad idea is futile. Craig pgpUi7TYwuAJK.pgp Description: PGP signature
Re: Please don't do this (code fragment)
On Mon, Jan 14, 2002 at 03:36:48PM +0100, Guus Sliepen wrote: On Mon, Jan 14, 2002 at 12:04:44AM -0800, [EMAIL PROTECTED] wrote: [How to write bad code] Why don't you man fts_read and implement the correct way of checking for errors? I believe the author wrote the original loop because the fts_ functions could fail to operate as expected. -- Met vriendelijke groet / with kind regards, Guus Sliepen [EMAIL PROTECTED]
Re: Please don't do this (code fragment)
On Mon, Jan 14, 2002 at 08:58:57AM -0800, [EMAIL PROTECTED] wrote: Why don't you man fts_read and implement the correct way of checking for errors? I believe the author wrote the original loop because the fts_ functions could fail to operate as expected. Well please check if the author's assumptions are valid, and if so, direct your attention to fixing fts_read(), everyone would benefit. The discussion of how to make that ugly loop look nicer is useless. -- Met vriendelijke groet / with kind regards, Guus Sliepen [EMAIL PROTECTED] pgpU56pFzfY5Y.pgp Description: PGP signature
Re: Please don't do this (code fragment)
Craig Dickson [EMAIL PROTECTED] writes: Richard Kettlewell wrote: Even if you don't care about weird platforms, x -1 is a ridiculously obscure test in this context; to achieve the same effect it would be much clearer to make x unsigned and do x = (unsigned)INT_MAX. I find x = (unsigned) INT_MAX to be more obscure than the original. When I first glanced at it, I thought, That's dumb, x is ALWAYS less than or equal to INT_MAX by definition! Then my second thought was that the cast on the constant will cause x to also be converted to unsigned. In contrast, when I saw x -1, I immediately realized it was testing for wraparound. By 'make x unsigned' I mean 'declare as unsigned int x;'. To achieve almost the same effect (probably close enough in this situation), it would be better to simply test for x INT_MAX -- it's clearer than either the original or your cast-dependent version, and only stops one iteration sooner. Since in this case the actual number of iterations was not the point (rather, merely that the loop should be guaranteed to terminate eventually), it ought to be sufficient. If we're allowed to change the meaning, there are much more sensible options still. That wasn't really the point of my post. -- http://www.greenend.org.uk/rjk/
Re: Please don't do this (code fragment)
Torsten Landschoff [EMAIL PROTECTED] writes: On Sun, Jan 13, 2002 at 10:23:51PM -0800, [EMAIL PROTECTED] wrote: Moreover, the original depends on the fact that (MAX_INT + 1) 0 which, though true, is a guaranteed result. Were I to be running a 64 ... is not a guaranteed ... I yet have to see a machine that does not use two's complement for integer arithmetic. Actually, the C standard does essentially guarantee two's complement arithmetic. It specifies integer overflow behavior and signed/unsigned conversion behavior exactly. So a machine can do its arithmetic some other way, but it has to make it seem to the programmer that it's always two's complement. Thomas
Re: Please don't do this (code fragment)
On Mon, Jan 14, 2002 at 06:01:37PM +0100, Guus Sliepen wrote: On Mon, Jan 14, 2002 at 08:58:57AM -0800, [EMAIL PROTECTED] wrote: Why don't you man fts_read and implement the correct way of checking for errors? I believe the author wrote the original loop because the fts_ functions could fail to operate as expected. Well please check if the author's assumptions are valid, and if so, direct your attention to fixing fts_read(), everyone would benefit. The discussion of how to make that ugly loop look nicer is useless. I think you are missing the point. As far as the program is concerned, there is no valid reason to use the protecting loop to guarantee termination. I, too, agree that there is no reason to make the loop nicer since there is no reason to write it in the first place. -- Met vriendelijke groet / with kind regards, Guus Sliepen [EMAIL PROTECTED]
Re: Please don't do this (code fragment)
On Mon, Jan 14, 2002 at 10:03:59AM -0800, Thomas Bushnell, BSG wrote: I yet have to see a machine that does not use two's complement for integer arithmetic. Actually, the C standard does essentially guarantee two's complement arithmetic. It specifies integer overflow behavior and signed/unsigned conversion behavior exactly. For real? Is that new with ISO C or was that in the original ANSI C standard as well? Sorry, I gave away my ANSI C book a while before and did not yet get it back :(( cu Torsten pgpWSabcVjFSJ.pgp Description: PGP signature
Re: Please don't do this (code fragment)
On Mon, Jan 14, 2002 at 03:51:18PM +0100, David N. Welton wrote: And what's more, it only indents two at a time. Heretics! From standards.info: For the body of the function, our recommended style looks like this: if (x foo (y, z)) haha = bar[4] + 5; else You want to discuss this on GNU lists? :) cu Torsten pgpoS9kmwtUFv.pgp Description: PGP signature
Re: Please don't do this (code fragment)
I don't find any such mention in KR [1978]. There is nothing I can see to guarantees that MAX_INT + 1 0. On Tue, Jan 15, 2002 at 12:56:19AM +0100, Torsten Landschoff wrote: On Mon, Jan 14, 2002 at 10:03:59AM -0800, Thomas Bushnell, BSG wrote: I yet have to see a machine that does not use two's complement for integer arithmetic. Actually, the C standard does essentially guarantee two's complement arithmetic. It specifies integer overflow behavior and signed/unsigned conversion behavior exactly. For real? Is that new with ISO C or was that in the original ANSI C standard as well? Sorry, I gave away my ANSI C book a while before and did not yet get it back :(( cu Torsten
