Could you check this little piece of C code?

[alpha123]

Bonus question: what is the type of a pointer that points to itself?

Um.... a useless pointer?!


I would just leave the code alone if it works and ignore any warnings .

Peter

[Jan Brouwer]

Bonus question: what is the type of a pointer that points to itself?

A void pointer can point to any type, so also to itself:

Code: Select all

void *p = &p;

What do I win?

[Zach Wegner]

Bonus pointers!

Good call though, I hadn't thought of that.

[michiguel]

It's a useless warning. It must refer to the evaluation of *scan and *match, which will not matter unless one of the pointers is pointing at the other pointer (which is what HG is saying). Most likely the compiler could tell if that was the case, though. The logical ANDs are sequence points, so that all of the side effects (i.e. increments) will be done before the next comparison.

Bonus question: what is the type of a pointer that points to itself?

Thanks to all guys, I will take a careful look at the messages tonight at home.

Yes, it is a silly warning in *this* particular case. The problem is that the warning might be useful in some other cases (according to Intel). In general, it is suppose to detect when there are dangerous side effects. In this case, there is no problem.

As it was suggested in the thread, I may choose to turn off the diagnostic, but that is not a good option. This is code that other people will use and I do not think I should be forcing them to turn warnings off. The other option is to introduce a #pragma and turn off the warning locally (I need to look at intel options for that). I did it with another one that the code was correct and there was no way I could fix it easily.

Miguel

[Dann Corbit]

The original code does 8 comparisons each time through the loop, while yours only does 7. Aside from that they look the same to me. I'm not sure what the performance change would be (I would guess little or nothing), but you can always test and find out.

Oh, crap, I cannot even count. Thanks, I knew it should be better to have another pair of eyes to check this. Particularly when it is late at night.

so the code should be this

Code: Select all

do {
	++scan; ++match; if (*scan != *match) break;
	++scan; ++match; if (*scan != *match) break;
	++scan; ++match; if (*scan != *match) break;
	++scan; ++match; if (*scan != *match) break;

	++scan; ++match; if (*scan != *match) break;
	++scan; ++match; if (*scan != *match) break;
	++scan; ++match; if (*scan != *match) break;
	++scan; ++match; if (*scan != *match) break;
} while (scan < strend);

Miguel

The original code is simply broken. Your code produces the correct result. The original code works by accident on some compilers.

The original code attempts to modify the variables scan and match multiple times between sequence points, so its behavior is undefined.

Here is a similar example from the C-FAQ:

Code: Select all

3.3b:	Here's a slick expression:

		a ^= b ^= a ^= b

	It swaps a and b without using a temporary.

A:	Not portably, it doesn't.  It attempts to modify the variable a
	twice between sequence points, so its behavior is undefined.

	For example, it has been reported that when given the code

		int a = 123, b = 7654;
		a ^= b ^= a ^= b;

	the SCO Optimizing C compiler (icc) sets b to 123 and a to 0.

	See also questions 3.1, 3.8, 10.3, and 20.15c.

See also:

Code: Select all

3.8:	How can I understand these complex expressions?  What's a
	"sequence point"?

A:	A sequence point is a point in time (at the end of the
	evaluation of a full expression, or at the ||, &&, ?:, or comma
	operators, or just before a function call) at which the dust
	has settled and all side effects are guaranteed to be complete.
	The ANSI/ISO C Standard states that

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

	The second sentence can be difficult to understand.  It says
	that if an object is written to within a full expression, any
	and all accesses to it within the same expression must be
	directly involved in the computation of the value to be written.
	This rule effectively constrains legal expressions to those in
	which the accesses demonstrably precede the modification.  For
	example, i = i + 1 is legal, but not a[i] = i++ (see question
	3.1).

	See also question 3.9 below.

	References: ISO Sec. 5.1.2.3, Sec. 6.3, Sec. 6.6, Annex C;
	Rationale Sec. 2.1.2.3; H&S Sec. 7.12.1 pp. 228-9.

3.9:	So given

		a[i] = i++;

	we don't know which cell of a[] gets written to, but i does get
	incremented by one, right?

A:	Not necessarily!  Once an expression or program becomes
	undefined, *all* aspects of it become undefined.  See questions
	3.2, 3.3, 11.33, and 11.35.

3.12a:	What's the difference between ++i and i++?

A:	If your C book doesn't explain, get a better one.  Briefly:
	++i adds one to the stored value of i and "returns" the new,
	incremented value to the surrounding expression; i++ adds one
	to i but returns the prior, unincremented value.

3.12b:	If I'm not using the value of the expression, should I use ++i
	or i++ to increment a variable?

A:	Since the two forms differ only in the value yielded, they are
	entirely equivalent when only their side effect is needed.
	(However, the prefix form is preferred in C++.)  See also
	question 3.3.

	References: K&R1 Sec. 2.8 p. 43; K&R2 Sec. 2.8 p. 47; ISO
	Sec. 6.3.2.4, Sec. 6.3.3.1; H&S Sec. 7.4.4 pp. 192-3, Sec. 7.5.8
	pp. 199-200.

[Dann Corbit]

P.S.
It would be good if you sent the original authors a defect report, because their code is broken.

[Dann Corbit]

I pasted too much. The 3.12 questions are not topical for this issue.

[Dann Corbit]

It's a useless warning. It must refer to the evaluation of *scan and *match, which will not matter unless one of the pointers is pointing at the other pointer (which is what HG is saying). Most likely the compiler could tell if that was the case, though. The logical ANDs are sequence points, so that all of the side effects (i.e. increments) will be done before the next comparison.

Bonus question: what is the type of a pointer that points to itself?

Thanks to all guys, I will take a careful look at the messages tonight at home.

Yes, it is a silly warning in *this* particular case. The problem is that the warning might be useful in some other cases (according to Intel). In general, it is suppose to detect when there are dangerous side effects. In this case, there is no problem.

As it was suggested in the thread, I may choose to turn off the diagnostic, but that is not a good option. This is code that other people will use and I do not think I should be forcing them to turn warnings off. The other option is to introduce a #pragma and turn off the warning locally (I need to look at intel options for that). I did it with another one that the code was correct and there was no way I could fix it easily.

Miguel

It's not a silly warning. The original code is utterly broken.

[Zach Wegner]

The original code is simply broken. Your code produces the correct result. The original code works by accident on some compilers.

The original code attempts to modify the variables scan and match multiple times between sequence points, so its behavior is undefined.

No. From the FAQ you posted:

A sequence point is a point in time (at the end of the evaluation of a full expression, or at the ||, &&, ?:, or comma operators, or just before a function call) at which the dust has settled and all side effects are guaranteed to be complete.

[Dann Corbit]

The original code is simply broken. Your code produces the correct result. The original code works by accident on some compilers.

The original code attempts to modify the variables scan and match multiple times between sequence points, so its behavior is undefined.

No. From the FAQ you posted:

A sequence point is a point in time (at the end of the evaluation of a full expression, or at the ||, &&, ?:, or comma operators, or just before a function call) at which the dust has settled and all side effects are guaranteed to be complete.

You are right that the result is not undefined.

However, it is not clear to me that the result is not unspecified.