QA-C gives side-effects warning. Unknown why

[Harry Ebbers]

Hi,

For some piece of code (an if-statement with multiple function-calls
to the same get-function in its condition) QA-C gives the following
warning on the 2 and further times the function is called:
"The right operand of '&&', '||', '?' or ':' has side effects, and
will only be executed under certain conditions. This is unclear. Try
using an explicit condition."
According to our coding-standard we should solve this QA-C warning.
But we want to understand first why this warning occurs.

And there lies the problem. We are breaking our head over why we get
this warning, since the function it calls is a get-function which does
not alterate any values. Note: the get-function is located in a
different module file than the if-statement.


The code looks like this:
(Note: SOME_TYPE is an enum-type)

Module_A.c

static SOME_TYPE some_arr[SIZE];

extern SOME_TYPE get_value(int index)
{
SOME_TYPE status = SOME_VALUE_0;

status = some_arr[index];

return status;
}

Module_B.c

#include <Module_A.h>

void function_with_QA_C_warning(void)
{
....
....
/* The lines marked with # are the lines where QA-C gives the
warning on */
if ((get_value(index1) == SOME_VALUE_1)
# || (get_value(index2) == SOME_VALUE_1)
# || (get_value(index3) == SOME_VALUE_1)
# || (get_value(index4) == SOME_VALUE_1))
{
....
}
....
}

Can anybody explain to us why QA-C sees these function-calls as calls
with side-effects?

Kind regards,

Harry Ebbers

 

[Willem]

Harry Ebbers wrote:
) Hi,
)
) For some piece of code (an if-statement with multiple function-calls
) to the same get-function in its condition) QA-C gives the following
) warning on the 2 and further times the function is called:
) "The right operand of '&&', '||', '?' or ':' has side effects, and
) will only be executed under certain conditions. This is unclear. Try
) using an explicit condition."

It doesn't say that the *function* has side effects, it tells you that the
*operand* has side effects. The operand, in this case, contains a function
call and in the mind of the compiler, function call implies side effect.

) Can anybody explain to us why QA-C sees these function-calls as calls
) with side-effects?

The compiler probably assumes that any function call has side effects.

Perhaps you can convince the compiler that that function does not have
side effects, with some compiler-specific extension.


SaSW, Willem
--
Disclaimer: I am in no way responsible for any of the statements
made in the above text. For all I know I might be
drugged or something..
No I'm not paranoid. You all think I'm paranoid, don't you !
#EOT

 

[Chris Dollin]

Can anybody explain to us why QA-C sees these function-calls as calls
with side-effects?

It's guessing. (Either that, or it does some clever analysis but gets it
wrong, or right but not useful; you might want to double-check the code
for `get_value` in case it does something like logging, a strangely
popular side-effect).

My advice: find out how to switch off the warning. (Perhaps there's a
way of annotating `get_value` with "No side effects here, honest, guv.".)
A C programmer that /doesn't/ know, and indeed exploit, that the right
operand of `||` will only be evaluated if the left operand is false, is
a C programmer with a learning opportunity.

If you can't switch the warning off, work out how you can filter it.

If you can't filter it, change compilers or change coding standards,
whichever is easier.

--
"It took a very long time, much longer than the most /Sector General/
generous estimates." - James White

Hewlett-Packard Limited registered no:
registered office: Cain Road, Bracknell, Berks RG12 1HN 690597 England

 

[Chris Dollin]

The code looks like this:
(Note: SOME_TYPE is an enum-type)

Module_A.c

static SOME_TYPE some_arr[SIZE];

extern SOME_TYPE get_value(int index)
{
SOME_TYPE status = SOME_VALUE_0;

status = some_arr[index];

return status;
}

How much like that does it look? Because if it looks a lot
like that, why doesn't it look more like

return some_arr[index];

? Initialising a status variable in its declaration, then
unconditionally assigning it a new value, and then returning
the value of the variable, ie the assigned value, is a long
way round the houses.

Even if you check that `index` is sane first, I can't think of a
good reason for the code to limp along like that. (Perhaps it
doesn't really look much like the code above.)

 

[Harry Ebbers]

The code looks like this:
(Note: SOME_TYPE is an enum-type)

Module_A.c

static SOME_TYPE some_arr[SIZE];

extern SOME_TYPE get_value(int index)
{
SOME_TYPE status = SOME_VALUE_0;

status = some_arr[index];

return status;
}

How much like that does it look?

It actually looks a bit more like:

Module_A.c

static SOME_TYPE some_arr[SIZE];

extern SOME_TYPE get_value(int index)
{
const char *func_name = "get_value";
SOME_TYPE status = SOME_VALUE_0;

status = some_arr[index];

Trace("<component_name>", INTERNAL, func_name, " index: %d, status:
%s", index, SOME_TYPE_2_STR(status));

return status;
}

Where Trace() is a function that sends some logging to a tracing-
process.

Thus there is some logging involved, which can be the cause of the
problem if I read your other post correctly.
I simpliefied it a bit, since other functions which are used in a
similar if-construction and which also use the Trace() function don't
get the QA-C warning.

Kind regards,

Harry

 

[Willem]

Harry Ebbers wrote:
) It actually looks a bit more like:
)
) Module_A.c
)
) static SOME_TYPE some_arr[SIZE];
)
) extern SOME_TYPE get_value(int index)
) {
) const char *func_name = "get_value";
) SOME_TYPE status = SOME_VALUE_0;
)
) status = some_arr[index];
)
) Trace("<component_name>", INTERNAL, func_name, " index: %d, status:
) %s", index, SOME_TYPE_2_STR(status));
)
) return status;
) }

Those look a hell of a lot like side effects to me.

But in any case, side effects occurring in short-circuited AND and OR
constructions are a often-used C idiom. There may be reasons for the
compiler to mention them, but insisting that every single such warning
is taken care of is ridiculous(1).
Perhaps there is a compiler-specific way to make it shut up about it.

) Thus there is some logging involved, which can be the cause of the
) problem if I read your other post correctly.
) I simpliefied it a bit, since other functions which are used in a
) similar if-construction and which also use the Trace() function don't
) get the QA-C warning.

I find that very strange, because logging is a definite side effect.


1) A personal gripe of mine is C code littered with '(void)' casts
meant to suppress the 'unused return value' warnings.


SaSW, Willem
--
Disclaimer: I am in no way responsible for any of the statements
made in the above text. For all I know I might be
drugged or something..
No I'm not paranoid. You all think I'm paranoid, don't you !
#EOT

 

[James Kuyper]

Hi,

For some piece of code (an if-statement with multiple function-calls
to the same get-function in its condition) QA-C gives the following
warning on the 2 and further times the function is called:
"The right operand of '&&', '||', '?' or ':' has side effects, and
will only be executed under certain conditions. This is unclear. Try
using an explicit condition."
According to our coding-standard we should solve this QA-C warning.
But we want to understand first why this warning occurs.

And there lies the problem. We are breaking our head over why we get
this warning, since the function it calls is a get-function which does
not alterate any values. Note: the get-function is located in a
different module file than the if-statement.

QA-C is a code checker that checks one module at a time. As a result,
while analyzing this code, it can't know whether or not get_value() has
any side-effects. The most conservative choice is to assume that
get_value() might have side effects.

/* The lines marked with # are the lines where QA-C gives the
warning on */
if ((get_value(index1) == SOME_VALUE_1)
# || (get_value(index2) == SOME_VALUE_1)
# || (get_value(index3) == SOME_VALUE_1)
# || (get_value(index4) == SOME_VALUE_1))

I used to have access to QA-C. The coding standards it enforces are
highly configurable; this warning can probably be turned off. However,
if your coding standard mandates that this particular feature of QA-C be
turned on, I can see the point. If the compiler can't be sure that
get_value() has no side-effects without looking outside of this module,
than neither can you.

If the contents of get_value() are simple enough, you might try
declaring it as 'static inline' and including the complete function
definition in the appropriate header file. Since I no longer have access
to QA-C, I can't verify this, but it seems plausible.

 

[Harry Ebbers]

Harry Ebbers wrote:

) It actually looks a bit more like:
)
) Module_A.c
)
) static SOME_TYPE some_arr[SIZE];
)
) extern SOME_TYPE get_value(int index)
) {
) const char *func_name = "get_value";
) SOME_TYPE status = SOME_VALUE_0;
)
) status = some_arr[index];
)
) Trace("<component_name>", INTERNAL, func_name, " index: %d, status:
) %s", index, SOME_TYPE_2_STR(status));
)
) return status;
) }

Those look a hell of a lot like side effects to me.

I checked if this Trace() function was the cause by commenting it out.
But the QA-C warnings still occured.

But in any case, side effects occurring in short-circuited AND and OR
constructions are a often-used C idiom. There may be reasons for the
compiler to mention them, but insisting that every single such warning
is taken care of is ridiculous(1).

That is why we want to understand why it is happening. "Don't fix it,
if it ain't broken" is our motto (I'm working on a sustaining
project).
But we have to take a look at it when there is an issue with the
coding-standard, since sometimes such a QA-C warning is showing a real
error (especially with the higher level warnings, and this is a level
4 one (lowest level that we have to fix)).
And when there is a reason to leave it in, an explanatory comment in
the code is also a "fix", since than people know why it is left in.

Perhaps there is a compiler-specific way to make it shut up about it.

Also here our motto applies.

) Thus there is some logging involved, which can be the cause of the
) problem if I read your other post correctly.
) I simpliefied it a bit, since other functions which are used in a
) similar if-construction and which also use the Trace() function don't
) get the QA-C warning.

I find that very strange, because logging is a definite side effect.

Seemingly it is not the cause of the side-effects (at least not
according to my experiment with commenting it out).

1) A personal gripe of mine is C code littered with '(void)' casts
meant to suppress the 'unused return value' warnings.

Indeed a nice way to introduce bugs, in the situation that a function
returned an error, which is not handled since it is cast to (void). >-
(

Kind regards,

Harry

 

[Stuart]

For some piece of code (an if-statement with multiple function-calls
to the same get-function in its condition) QA-C gives the following
warning on the 2 and further times the function is called:
"The right operand of '&&', '||', '?' or ':' has side effects, and
will only be executed under certain conditions. This is unclear. Try
using an explicit condition." ....
We are breaking our head over why we get
this warning, since the function it calls is a get-function which does
not alterate any values. Note: the get-function is located in a
different module file than the if-statement.

I don't know QA-C or the depth to which it does this type of analysis (my
experience is with Ada/SPARK).

t would not be surprising if the get-function is modifying an input stream
and this is the side effect that is being referred to. In this case the
warning is that it is 'unclear' as to what state the input stream will end
up being.

I believe the reason it is labelled as being 'unclear' is because it is not
certain whether you appreciated the existence of the side-effect and the
consequences of '||'. If it had been written long-hand as a nested series
of 'ifs' the assumption would [probably] be that you were aware of the
side-effects.

Regards
Stuart

 

[James Kuyper]

Seemingly it is not the cause of the side-effects (at least not
according to my experiment with commenting it out).

What you've proven is that the logging code is not the cause of the
warnings about side-effects. That's not the same a proving that it isn't
a side effect.

The C standard says:
"Accessing a volatile object, modifying an object, modifying a file, or
calling a function that does any of those operations are all _side
effects_." (5.1.2.3p2). I use the underscores to indicate that "side
effects" is in italic type. That's the standard's way of indicating that
this sentence constitutes the definition of a side effect. Writing to a
log file is definitely "modifying a file", and therefore unambiguously a
side effect.

Commenting out the logging code didn't have any effect on the warnings
you got, because those warnings were not based upon an an analysis of
the code for get_value(). Those warnings were based upon a worst-case
analysis that assumed, since QA-C didn't know what get_value() does,
that the function call might be a side effect.

 

[Chris Dollin]

I believe the reason it is labelled as being 'unclear' is because it is not
certain whether you appreciated the existence of the side-effect and the
consequences of '||'.

I think the reason it is labelled "unclear" is that the writers of the message
were pessimistic about the quality of the programmers who would be asking
for the warnings. (I don't know whether their pessimism is justified.)

The behaviour of `a || b || ... || z` is perfectly clear once you've learned
it, just like the behaviour of `=`, `while`, and `+`. I'll grant that the
behaviour of `a || b && c` is less clear, but that wasn't the code that
was being diagnosed.

 

[Harry Ebbers]

What you've proven is that the logging code is not the cause of the
warnings about side-effects. That's not the same a proving that it isn't
a side effect.

You're right. I should have phrased that better.
I was trying to rule it out as cause for the observed QA-C warnings
and succeeded in that, but in nothing more.

I did another experiment by copying the function to the module on
which QA-C gave the warnings (and using that local version of the
function i.s.o. the original function), just to check if that would
solve it.
But alas, the warnings stayed.

Anyway in the meantime we have decided to leave it in for now and
we've sent a mail to the deparment who made our coding standard, to
see if they will come up with a solution. (E.g. change the error-text
from "has side-effects" to "may have side-effects" and some
clarification that this is also true for *any* function implemented in
a different module).

(A "solution" in this case would have been: assign the results of the
function to some temp variables and use those variables in the if, but
that sounds more like tool-satisfaction and therefore we do/did not
want to do that (yet). (Better a real solution than just doing
something just to satify a tool IMHO))

Kind regards,

Harry

 

[CBFalconer]

You will get a similar warning from PC Lint, for the same reason.
The analysis tool assumes that the function has side effects, and
depending on run time values, you may call it one, two, three, or
four times.

With PC Lint, you could inform the tool that the function
get_value() does not have side effects. It's been quite a while
since I used QA-C, but most likely they have something similar.

Errors with short-circuited side effects due to || and && do
happen, although they are not terribly common in my experience.
So the warning is not without value.

I think the point being missed by the OP, and not really being made
by the answers, is that the '||' code evaluates the operand, and
avoids the rest of the statement if the answer is true. Thus, in
the above, if "(get_value(index1) == SOME_VALUE_1)" returns a true
all the other get_values will not be executed. If the OP wants to
execute all those get_value calls, he should replace '||' with
'|'. As it is, the first one to return a true settles the
condition being tested by the if.

I think the error message returned is confusing to the user.

 

[Thad Smith]

Hi,

For some piece of code (an if-statement with multiple function-calls
to the same get-function in its condition) QA-C gives the following
warning on the 2 and further times the function is called:
"The right operand of '&&', '||', '?' or ':' has side effects, and
will only be executed under certain conditions. This is unclear. Try
using an explicit condition."
According to our coding-standard we should solve this QA-C warning. ....
void function_with_QA_C_warning(void)
{
....
....
/* The lines marked with # are the lines where QA-C gives the
warning on */
if ((get_value(index1) == SOME_VALUE_1)
# || (get_value(index2) == SOME_VALUE_1)
# || (get_value(index3) == SOME_VALUE_1)
# || (get_value(index4) == SOME_VALUE_1))
{
....
}
....
}

There are two ways I can think of that would probably avoid the warning:

if ((get_value(index1) == SOME_VALUE_1)
| (get_value(index2) == SOME_VALUE_1)
| (get_value(index3) == SOME_VALUE_1)
| (get_value(index4) == SOME_VALUE_1))
{
....
}

That will work, but always calls get_value 4 times, rather than stopping
with the first match of SOME_VALUE_1.

The other is

if (get_value(index1) != SOME_VALUE_1);
else if (get_value(index2) != SOME_VALUE_1);
else if (get_value(index3) != SOME_VALUE_1);
else if (get_value(index4) != SOME_VALUE_1);
else
{
....
}

That is harder for me to read. I prefer your original code.

 

[Thad Smith]

Errors with short-circuited side effects due to || and && do happen,
although they are not terribly common in my experience. So the
warning is not without value.

Yes, the warnings have value, but once a warned item is examined and
found benign, it can be used. I believe PC-Lint supports commands added
in a comment to disable the warning locally.

A coding standard, especially one with no easy means of accepting warned
but validated code, can force the programmer into less-readable or
less-efficient work arounds.

 

[Keith Thompson]

[...]
I think the point being missed by the OP, and not really being made
by the answers, is that the '||' code evaluates the operand, and
avoids the rest of the statement if the answer is true.

Um, I don't think anybody missed that. (And it avoids the rest of the
expression, not the rest of the statement.)

Thus, in
the above, if "(get_value(index1) == SOME_VALUE_1)" returns a true
all the other get_values will not be executed. If the OP wants to
execute all those get_value calls, he should replace '||' with
'|'. As it is, the first one to return a true settles the
condition being tested by the if.

Replacing "||" by "|" seems like a really bad idea, just as a matter
of style if nothing else. Even ignoring the short-circuit behavior,
they mean different things; "||" is a logical or, and "|" is a bitwise
or.

Now it happens that all the operands are "==" comparisons, which are
guaranteed to yield either 0 or 1, so the result will be the same *in
this case*. But future maintenance could easily replace one of the
expressions with a function call or variable reference that can use a
non-zero value other than 1 to represent a true result. And anyone
reading the code would have to go through the same reasoning to
understand it -- or might assume that "|" is a typo for "||" and "fix"
it.

If I wanted to avoid the warning and guarantee that all the
subexpressions are evaluated, I'd much rather write something like
this:

const int result1 = get_value(index1) == SOME_VALUE_1;
const int result2 = get_value(index2) == SOME_VALUE_2;
const int result3 = get_value(index3) == SOME_VALUE_3;
const int result4 = get_value(index4) == SOME_VALUE_4;

if (result1 || result2 || result3 || result4) {
/* ... */
}

Depending on the code, assigning names to the subexpressions might
even make it clearer.

(I *might* consider writing a macro like:
#define OR4(x1, x2, x3, x4) (!!(x1) | !!(x2) | !!(x3) | !!(x4))
but I wouldn't consider it for very long.)

The OP mentioned elsethread that get_value() writes trace messages to
a log somewhere, so changing the code so it's always called 4 times
really does change the behavior. He has to decide whether that
matters.

 

[Ben Bacarisse]

Harry Ebbers wrote:

There are two ways I can think of that would probably avoid the warning:

if ((get_value(index1) == SOME_VALUE_1)
| (get_value(index2) == SOME_VALUE_1)
| (get_value(index3) == SOME_VALUE_1)
| (get_value(index4) == SOME_VALUE_1))
{
....
}

That will work, but always calls get_value 4 times, rather than
stopping with the first match of SOME_VALUE_1.

The other is

if (get_value(index1) != SOME_VALUE_1);
else if (get_value(index2) != SOME_VALUE_1);
else if (get_value(index3) != SOME_VALUE_1);
else if (get_value(index4) != SOME_VALUE_1);
else
{
....
}

That is harder for me to read. I prefer your original code.

Your second re-write is not the same thing at all! It is a form of

if ((get_value(index1) == SOME_VALUE_1)
&& (get_value(index2) == SOME_VALUE_1)
&& (get_value(index3) == SOME_VALUE_1)
&& (get_value(index4) == SOME_VALUE_1))
{
....
}

 

[Harry Ebbers]

I think the point being missed by the OP, and not really being made
by the answers, is that the '||' code evaluates the operand, and
avoids the rest of the statement if the answer is true. Thus, in
the above, if "(get_value(index1) == SOME_VALUE_1)" returns a true
all the other get_values will not be executed. If the OP wants to
execute all those get_value calls, he should replace '||' with
'|'. As it is, the first one to return a true settles the
condition being tested by the if.

I think the error message returned is confusing to the user.

You are wrong. I did not miss this point.

It is very clear to me that on an && the rest of the condition is not
evaluated when the first part is FALSE, since the result of the &&
will be FALSE anyway and with an || the rest is not evaluated when the
first part is TRUE since the result off the || will be TRUE anyway.

Since it was clear to me, I did not mention it in my original post,
there was no need to.

Please do not make assumptions, but keep to facts. In doubt ask, but
don't assume.

Kind regards,

Harry

 

[Harry Ebbers]

Your second re-write is not the same thing at all! It is a form of

if ((get_value(index1) == SOME_VALUE_1)
&& (get_value(index2) == SOME_VALUE_1)
&& (get_value(index3) == SOME_VALUE_1)
&& (get_value(index4) == SOME_VALUE_1))
{
....
}

The other solution would be:

/* we have a implementation for type bool */
bool temp_val1 = (get_value(index1) == SOME_VALUE_1);
bool temp_val2 = (get_value(index2) == SOME_VALUE_1);
bool temp_val3 = (get_value(index3) == SOME_VALUE_1);
bool temp_val4 = (get_value(index4) == SOME_VALUE_1);

if (temp_val1 || temp_val2 || temp_val3 || temp_val4)
{
....
}

But, like I already said else where in this thread, this is - in this
case - tool satisfaction in our (not only my) opinion, which actually
could have had unwanted side-effects in cases where the results for
the different indexes would have been mutual exclusive. (Although in
that case an if - elseif construction with explanatiory comment would
have been a better readable / clearer and thus safer implementation-
form, eventhough that would mean more lines of code)

Kind regards,

Harry

 

[Thad Smith]

Your second re-write is not the same thing at all! It is a form of

if ((get_value(index1) == SOME_VALUE_1)
&& (get_value(index2) == SOME_VALUE_1)
&& (get_value(index3) == SOME_VALUE_1)
&& (get_value(index4) == SOME_VALUE_1))
{
....
}

Hmmm, bad things happen to me when my code is hard to read. Thanks for
catching that.

Here's an even more perverse version (still not tested):

do
{
if (get_value(index1) != SOME_VALUE_1)
if (get_value(index2) != SOME_VALUE_1)
if (get_value(index3) != SOME_VALUE_1)
if (get_value(index4) != SOME_VALUE_1) break;
/* Do something if any value equal */
} while (0);