Could you check this little piece of C code?

[michiguel]

I am trying to silence compiler warnings of the compression libraries for the Gaviota TBs (Zlib in this case).
I think it is important, so the people won't have to turn off their warnings when they incorporate foreign code. I am really close to achieve that, by casting many stupid conversions. However, there is one piece of code that needs to be rewritten. I hate to do that with tested bug-free code (Zlib), but if I change this, the whole set of APIs will compile cleanly with the most strict warnings of the intel compiler. Since I am paranoid, could you double check that I am not screwing anything up? Several people may end up using it.

This is the code

Code: Select all

		/* We check for insufficient lookahead only every 8th comparison;
		 * the 256th check will be made at strstart+258.
		 */
		do {
		} while(
				*++scan == *++match && *++scan == *++match &&
				*++scan == *++match && *++scan == *++match &&
				*++scan == *++match && *++scan == *++match &&
				*++scan == *++match && *++scan == *++match &&
				scan < strend);

I am planning to comment it out and replace it with

Code: Select all

		/*MAB: to silence compiler warning, intel 981*/
		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;
		} while (scan < strend);

I cannot see why it should be slower. It is more verbose but it is more straightforward and 8 warnings are gone (#981: operands are evaluated in unspecified order).

Miguel

[Aaron Becker]

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.

[michiguel]

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

[Sven]

Do you still get the same warnings (remarks) with this version?

Code: Select all

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

If yes then the operands being "evaluated in unspecified order" are most probably the operands of the == resp. != operator. Otherwise I would think that the && operands are meant here.

I would not expect any problem related to the "*++scan" type of expression, at least.

What puzzles me is that both C and C++ standards specify exactly the order of evaluation (left-to-right) and operator precedence (pre-increment then dereference then == then &&) for your original piece of code:

1. perform ++scan
2. apply * to the result of ++scan
3. perform ++match
4. apply * to the result of ++match
5. compare both results of * with ==
6. if == evaluates to non-zero then repeat the same for the next (*++scan == *++match) expression
7. apply && to both results of ==
and so on ...

So according to the standard I can't see any possibility of an "unspecified order", which leads me to the conclusion that the Intel compiler gives a remark which is either kind of "wrong", or indicates that Intel generates dubious code here. Since you state that your code is tested and works fine, I would assume the former and would therefore simply suppress that compiler remark with an appropriate option (maybe "-wd981"). This looks better for me than changing correct code based on a dubious compiler remark only.

Sven

[bob]

I am trying to silence compiler warnings of the compression libraries for the Gaviota TBs (Zlib in this case).
I think it is important, so the people won't have to turn off their warnings when they incorporate foreign code. I am really close to achieve that, by casting many stupid conversions. However, there is one piece of code that needs to be rewritten. I hate to do that with tested bug-free code (Zlib), but if I change this, the whole set of APIs will compile cleanly with the most strict warnings of the intel compiler. Since I am paranoid, could you double check that I am not screwing anything up? Several people may end up using it.

This is the code

Code: Select all

		/* We check for insufficient lookahead only every 8th comparison;
		 * the 256th check will be made at strstart+258.
		 */
		do {
		} while(
				*++scan == *++match && *++scan == *++match &&
				*++scan == *++match && *++scan == *++match &&
				*++scan == *++match && *++scan == *++match &&
				*++scan == *++match && *++scan == *++match &&
				scan < strend);

I am planning to comment it out and replace it with

Code: Select all

		/*MAB: to silence compiler warning, intel 981*/
		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;
		} while (scan < strend);

I cannot see why it should be slower. It is more verbose but it is more straightforward and 8 warnings are gone (#981: operands are evaluated in unspecified order).

Miguel

The warnings are caused by all the ++scans.

Why not replace each instance of *++scan with
*(scan+n) where N is 0, 1, ...

and when done, increment scan as needed? That'll solve the warnings and let you keep your original code, assuming it is faster. To determine why something is faster or slower, I always use the -S option so that I can see the asm.

[Bo Persson]

Do you still get the same warnings (remarks) with this version?

Code: Select all

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

If yes then the operands being "evaluated in unspecified order" are most probably the operands of the == resp. != operator. Otherwise I would think that the && operands are meant here.

I would not expect any problem related to the "*++scan" type of expression, at least.

What puzzles me is that both C and C++ standards specify exactly the order of evaluation (left-to-right) and operator precedence (pre-increment then dereference then == then &&) for your original piece of code:

1. perform ++scan
2. apply * to the result of ++scan
3. perform ++match
4. apply * to the result of ++match
5. compare both results of * with ==
6. if == evaluates to non-zero then repeat the same for the next (*++scan == *++match) expression
7. apply && to both results of ==
and so on ...

So according to the standard I can't see any possibility of an "unspecified order", which leads me to the conclusion that the Intel compiler gives a remark which is either kind of "wrong", or indicates that Intel generates dubious code here. Since you state that your code is tested and works fine, I would assume the former and would therefore simply suppress that compiler remark with an appropriate option (maybe "-wd981"). This looks better for me than changing correct code based on a dubious compiler remark only.

Sven

I guess you are thinking about Java?

In C and C++ there is definitely no left-to-right evaluation order in an expression. Operations 3 and 4 could easily be performed before ops 1 and 2. Or in between.

On the other hand, I believe the compiler issues this remark for any code which has more than one possible evaluation order, even when the order doesn't matter.

I have turned it off for my code.

[Michel]

In C and C++ there is definitely no left-to-right evaluation order in an expression. Operations 3 and 4 could easily be performed before ops 1 and 2. Or in between.

For boolean expressions there should be. This lets you write something
like

p && (a==*p);

If p is a null pointer then the second part of the expression is not evaluated.

[jwes]

I am trying to silence compiler warnings of the compression libraries for the Gaviota TBs (Zlib in this case).
I think it is important, so the people won't have to turn off their warnings when they incorporate foreign code. I am really close to achieve that, by casting many stupid conversions. However, there is one piece of code that needs to be rewritten. I hate to do that with tested bug-free code (Zlib), but if I change this, the whole set of APIs will compile cleanly with the most strict warnings of the intel compiler. Since I am paranoid, could you double check that I am not screwing anything up? Several people may end up using it.

This is the code

Code: Select all

		/* We check for insufficient lookahead only every 8th comparison;
		 * the 256th check will be made at strstart+258.
		 */
		do {
		} while(
				*++scan == *++match && *++scan == *++match &&
				*++scan == *++match && *++scan == *++match &&
				*++scan == *++match && *++scan == *++match &&
				*++scan == *++match && *++scan == *++match &&
				scan < strend);

I am planning to comment it out and replace it with

Code: Select all

		/*MAB: to silence compiler warning, intel 981*/
		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;
		} while (scan < strend);

I cannot see why it should be slower. It is more verbose but it is more straightforward and 8 warnings are gone (#981: operands are evaluated in unspecified order).

Miguel

You could try this - it looks more like the original code:

Code: Select all

		/* We check for insufficient lookahead only every 8th comparison;
		 * the 256th check will be made at strstart+258.
		 */
		do {
		} while(
				(*++scan == *++match) && (*++scan == *++match) &&
				(*++scan == *++match) && (*++scan == *++match) &&
				(*++scan == *++match) && (*++scan == *++match) &&
				(*++scan == *++match) && (*++scan == *++match) &&
				scan < strend);

[hgm]

I guess in theory it is possible that scan ponts to match, or vice versa:

Code: Select all

scan = &match;
match = &match;
if(*scan++ == *match++) ...

Now what value *scan and *match have will depend on if they are evauated before or after match is incremented.

[Zach Wegner]

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?