I came across a comment in cppcheck source code here, stating that it is unsafe to move the definition of i in the example into the inner loop. Why is that?

void CheckOther::variableScopeError(const Token *tok, const std::string &varname)
 {
     reportError(tok,
                 Severity::style,
                 "variableScope",
                 "$symbol:"   varname   "\n"
                 "The scope of the variable '$symbol' can be reduced.\n"
                 "The scope of the variable '$symbol' can be reduced. Warning: Be careful "
                 "when fixing this message, especially when there are inner loops. Here is an "
                 "example where cppcheck will write that the scope for 'i' can be reduced:\n"
                 "void f(int x)\n"
                 "{\n"
                 "    int i = 0;\n"
                 "    if (x) {\n"
                 "        // it's safe to move 'int i = 0;' here\n"
                 "        for (int n = 0; n < 10;   n) {\n"
                 "            // it is possible but not safe to move 'int i = 0;' here\n"
                 "            do_something(&i);\n"
                 "        }\n"
                 "    }\n"
                 "}\n"
                 "When you see this message it is always safe to reduce the variable scope 1 level.", CWE398, Certainty::normal);
 }

CodePudding user response：

Why is that?

The following code outputs i=10:

#include <cstdio>

void save_life(int i) {
    printf("i=%d\n", i);
}

void do_something(int *i) {
    *i  = 1;
    if (*i == 10) {
        save_life(*i);
    }
}

 void f(int x)
 {
    int i = 0;
    if (x) {
        for (int n = 0; n < 10;   n) {
            do_something(&i);
        }
    }
 }

 int main() {
     f(1);
 }

However, the following code doesn't output anything:

#include <cstdio>

void save_life(int i) {
    printf("i=%d\n", i);
}

void do_something(int *i) {
    *i  = 1;
    if (*i == 10) {
        save_life(*i);
    }
}

 void f(int x)
 {
    if (x) {
        for (int n = 0; n < 10;   n) {
            int i = 0;
            do_something(&i);
        }
    }
 }

 int main() {
     f(1);
 }

Because do_something internal state may depend on the pointed-to-value, it's not "safe" to move the declaration in the loop. The function may depend on the state from previous iteration.

CodePudding user response：

The question is focused on the second comment in this hypothetical code:

void f(int x)
    //int i = 0;
    if (x) {
        // it's safe to move 'int i = 0;' here
        for (int n = 0; n < 10;   n) {
             int i=0;
             // it is possible but not safe to move 'int i = 0;' here
             do_something(&i);
        }
    }
}

There no general issue taking the address of a variable declared in a loop so long as that address (so taken) is not dereferenced after the loop:

int *p=nullptr;
for(int n=0;n<10;  n){
    int i=0;
    p=&i; 
    //can use value of p here.
}
//address of i assigned to p now invalid.

This is the same general notion of variable scope that means pointers to local variables can't be used after the function that defined them returns(*). In this case in anything but the iteration the address was taken.

int *q=nullptr;
for(int n=0;n<10;  n){
    int i=0;
    if(n==0){
        q=&i;
    }else{
        //cannot use q here. 
        //it is trying to save the address from the first iteration.
    }
}

So it's not clear what the original author's concern is. What is clear in the code as presented is i will be 0 when do_something(&i) is called every time and in the code as shown nothing is done with any value do_something() assigns to the variable through its pointer.

Sometimes it is necessary to provide an address to a general purpose function that the specific caller doesn't then need to use.

But the code provided isn't sufficient to explain the comment.

(*) For the same reasons that the implementation may have released or reused the memory for other purposes as it leaves the relevant scope.