S886: Is this rule wrong?

[Oodini (Robert)]

Hello,

I have a problem with the following code (sorry, the preformatted tags make the code unreadable on Firefox):

bool found=false;
for (XPixelCoord localOffset = 0; (!found) && (localOffset < halfKnotsSpacing); ++localOffset)
{
	{
		const XPixelCoord candidateIndex = currentKnotPixelIndex - localOffset;
		if ( _contour[candidateIndex] != 0)
		{
			contourAsCoords[currentKnotIndex] = buildPoint2D(candidateIndex, _contour[candidateIndex]);
			found = true;
		}
	}
}

I get the error S886:
The three expressions of a “for” statement should only be concerned with loop control

Sonar is annoyed by the found in the condition:

This rule is inspired by MISRA:
MISRA C++:2008, 6-5-5 - A loop-control-variable other than the loop-counter shall not be modified within condition or expression.

But the first sample code given by MISRA for this rule is very similar to my code:

for (x=0; (x<10) && !bool_a; ++x)
{
    if (...)
    {
        bool_a = true;    // Compliant
    }
}

So I don’t understand why it is a problem for Sonar.

[JolyLoic (Loïc Joly)]

Hi @Oodini,

I agree that your code is compliant with MISRA C++2008. While S886 is inspired by MISRA, it is not s strict implementation of MISRA (otherwise, it would have the tag misra-c++2008, not based-on-misra).

The purpose of this rule is to ensure that for-loops are dead simple to read, because experience has shown that they are usually error-prone. So, while MISRA C++2008 allows loop-control-variables (which we call Pseudo-Counter in our rule) other than the loop-counter, we decided not to. We consider that alternative ways to write the loop are usually more explicit and readable, for instance, in your case:

for (XPixelCoord localOffset = 0; localOffset < halfKnotsSpacing; ++localOffset)
{
	{
		const XPixelCoord candidateIndex = currentKnotPixelIndex - localOffset;
		if ( _contour[candidateIndex] != 0)
		{
			contourAsCoords[currentKnotIndex] = buildPoint2D(candidateIndex, _contour[candidateIndex]);
			break;
		}
	}
}

If you are interested in MISRA compliance, we have rules that directly cover the for-loops part of MISRA-C++:2008: S890, S892, S5311, S5312, S5313, S5316.

Note that this part of MISRA has been extensively reworked in MISRA C++:2023, and that your initial code would not be compliant with the new rule 9.5.1, while my suggested rewrite would be (assuming that XPixelCoord is an integer type). We do not yet provide direct support for MISRA C++2023 rule 9.5.1.

[Oodini (Robert)]

Thanks @JolyLoic for your very complete answer.

I had already tried what you suggest, but then I had a problem with the rule S1227: break statements should not be used except for switch cases. This one is not inspired by any standard.

So we have two Sonar rules which are incompatible ?

[JolyLoic (Loïc Joly)]

Yes, we do have some contradictions in our rules because there are different conflicting points of view in the C++ community (on almost any topic…).

We did S1227 because some people seem to think it has some value (as you may have guessed, I’m not one of those; I think it actually decreases the quality of the code). And because, to say the least, it is controversial, it is not part of our default quality profile.

My suggestion here would be to disable S1227. If it was not already in the product and we were considering adding it, I would be fighting against it. However, the bar to remove something that is already present is higher than the bar to reject it in the first place.

[Oodini (Robert)]

@JolyLoic

there are different conflicting points of view in the C++ community (on almost any topic…)

Thank you for your help !

[JolyLoic (Loïc Joly)]

For information, after your message, we discussed more on rule S1227, and decided to deprecate it.