#include <map>
const std::map<int, int> my_map{{10, 20}};
const int index = 10;
for (int i; i < my_map.at(index); i++) {}
Hi @bers,
Thank you for the report. I fully agree that the value my_map.at(index) is constant and does not change between the loop iterations. However, this condition will still be evaluated before entering each iteration of the loop, thus my_map.at(index) will be invoked multiple times. This is why this rule raises an issue with the message:
Calculate the stop condition value outside the loop and set it to a variable.
To suggest the following rewrite*, that will evaluate my_map.at(index) only once regardless of the number of iterations of the loop.
#include <map>
const std::map<int, int> my_map{{10, 20}};
const int index = 10;
const int stop = my_map.at(index);
for (int i = 0; i < stop; i++) {}
* I have also corrected the loop index to be properly initialized, otherwise, this loop has undefined behavior.
@Tomasz_Kaminski, thanks for your explanations!
I have two follow-up comments, now that I have seen that the rule is defined based on the stop condition depending on some “function call” (which makes me agree that the implementation is correct, but now I question the definition of the rule itself):
I am not quite sure what the problem is with “invoking the condition multiple times” in itself. With a constant object, I’d expect the compiler to be smart enough to re-use this result, so it’s probably not related to performance. What else is it, then?
I feel this rule may require some further refinement. In fact, in the below snippet, the first loop raises cpp:S127, while the second one does not - is calling operator[] not a “function call”? Even worse, in the first loop the stop condition is not seen as constant while it is; in the second loop, the stop condition is actually non-constant.
#include <map>
const int index = 10;
// Loop 1
const std::map<int, int> my_constant_map{{10, 20}};
for (int i = 0; i < my_constant_map.at(index); i++) {}
// Loop 2
std::map<int, int> my_non_constant_map{{10, 20}};
for (int i = 0; i < my_non_constant_map[index]; i++) {}
- I am not quite sure what the problem is with “invoking the condition multiple times” in itself. With a constant object, I’d expect the compiler to be smart enough to re-use this result, so it’s probably not related to performance. What else is it, then?
This is generally not the case, as this would require the compiler to inline all the calls of the function, to provide that despite returning the same value it does not have any side effects. Such side effects, include but are not limited to printing logs, updating global variables, or just changing members that is mutable.
In addition, any such optimization is highly dependent on the context of the call, so it may happen for a small example, but not in a larger context. Extracting such a function guarantees that the condition is evaluated only once, and conveys the fact that loop condition is not modified in the loop to the reader.
- I feel this rule may require some further refinement. In fact, in the below snippet, the first loop raises
cpp:S127, while the second one does not - is callingoperator[]not a “function call”? Even worse, in the first loop the stop condition is not seen as constant while it is; in the second loop, the stop condition is actually non-constant.
In C++ the expression my_non_constant_map[index] will either return a value associated for the index if it is present in my_non_constant_map or will assign value-initialized (in case of int 0) object to the map. As a consequence, this operation is considered to be mutating, and cannot be extracted to be computed outside of the condition in the general case, without knowing the exact content of the my_non_constant_map and assuring that it is not modified by any loop iteration*. This is why this rule does not suggest to extract this call.
This is why operator[] is not allowed to be invoked on constant map objects in C++.
* If map you are using is a reference parameter it may refer to a global object, and thus may be modified by any function in the code. Similarly, if you pass the local mutable map object by reference to any function, its content may be mutated or a pointer to it may be stored to a global variable leading to same effect.
Thanks, I understand. Final set of questions, I hope: if the compiler is not allowed to do these optimizations even in the const case due to potential side effects, why does SonarLint propose these changes then? One reason might be “because SonarLint knows that map::at() does not have side effects, and moving the stop condition out of the loop will not change runtime behavior” - is that correct?
I also understand the reasoning for your answer to my second question, but find it at odds with the definition of the rule, which says it will flag stop conditions using a method call. First, it does not do that here. Second, the reason why it’s not flagged sounds weird to me: I understand that you are saying that SonarLint knows that map.operator[]() has side effects - but wouldn’t that be an extra-strong reason to flag this stop conditions? After all, the rule aims to avoid stop conditions “being difficult to understand and maintain”, and that is much more true in the second case than in the first.
Either way, I think the wording of the rule should be modified to match the implementation, and possibly also to further clarify its intent (especially in terms of breaking or not breaking run-time behavior, and regarding side effects).
Hi,
My apologies, I have misread the rule details previously, and it indeed raises the issue if the condition contains any function call, including calling operator[] and the rule behaves accordingly and raises the example you have listed as far as I can tell (godbolt link):
std::map<int, int> my_non_constant_map{{10, 20}};
for (int i = 0; i < my_non_constant_map[index]; i++) {}
This rule suggests rewriting the loop to avoid any function calls, and it is left to the programmer to perform it in a way that would make the behavior of the program equivalent. Note that this rule is based on the (older) MISRA standard, and is not enabled in the default SonarWay profile. That means we believe that it may be valuable for some projects, that could opt-in, but we do not think it should be enabled by default for all projects.
Responding to your question in more general tool context.
The compiler itself is bound by the standard, that requires that the program produced from the code behave the same way, as it was written*. If you have written a loop condition that calls a function it is required to call the function, unless it can prove that the behavior of the program, that can be observed in any way will not change.
This is however different from how humans and our tools interpret the code. The usual exceptation of the programmer for well-designed class is that the const method invoked will not have meaningful effects on the primary output of the program. It may affect some trace or log data, but such changes are acceptable. As the tool that helps with code review, we follow similar assumptions for other rules.
* Except few exceptions as NVRO (named return optimization) or more prominently optimization related to Undefined Behavior, where the compiler can assume that certain outcomes (like null pointer dereference) are not possible.
Maybe that is because your most recent example raises cpp:S886, while the topic is about cpp:S127. Nevermind, though - thanks for the very details explanations 
Indeed the current situation of rspec and implementation is very confusing, I have created the following ticket to summarize and track the issue CPP-4987 S127 Synchronize the description and implementation. As this is not SonarWay rule, it is a lower priority, and we do not have an explicit plan to work on it soon.
This topic was automatically closed 7 days after the last reply. New replies are no longer allowed.