Declaring variables inside loops, good practice or bad practice?
Asked Answered
T

9

399

Question #1: Is declaring a variable inside a loop a good practice or bad practice?

I've read the other threads about whether or not there is a performance issue (most said no), and that you should always declare variables as close to where they are going to be used. What I'm wondering is whether or not this should be avoided or if it's actually preferred.

Example:

for(int counter = 0; counter <= 10; counter++)
{
   string someString = "testing";

   cout << someString;
}

Question #2: Do most compilers realize that the variable has already been declared and just skip that portion, or does it actually create a spot for it in memory each time?

Tapping answered 31/10, 2011 at 20:50 Comment(3)
Put them close to their usage, unless profiling says otherwise.Allegedly
Here are some similar questions: #983463 #407755Hausner
@drnewman I did read those threads, but they didn't answer my question. I understand that declaring variables inside loops works. I'm wondering if it's a good practice to do so or if it's something to be avoided.Tapping
S
500

This is excellent practice.

By creating variables inside loops, you ensure their scope is restricted to inside the loop. It cannot be referenced nor called outside of the loop.

This way:

  • If the name of the variable is a bit "generic" (like "i"), there is no risk to mix it with another variable of same name somewhere later in your code (can also be mitigated using the -Wshadow warning instruction on GCC)

  • The compiler knows that the variable scope is limited to inside the loop, and therefore will issue a proper error message if the variable is by mistake referenced elsewhere.

  • Last but not least, some dedicated optimization can be performed more efficiently by the compiler (most importantly register allocation), since it knows that the variable cannot be used outside of the loop. For example, no need to store the result for later re-use.

In short, you are right to do it.

Note however that the variable is not supposed to retain its value between each loop. In such case, you may need to initialize it every time. You can also create a larger block, encompassing the loop, whose sole purpose is to declare variables which must retain their value from one loop to another. This typically includes the loop counter itself.

{
    int i, retainValue;
    for (i=0; i<N; i++)
    {
       int tmpValue;
       /* tmpValue is uninitialized */
       /* retainValue still has its previous value from previous loop */

       /* Do some stuff here */
    }
    /* Here, retainValue is still valid; tmpValue no longer */
}

For question #2: The variable is allocated once, when the function is called. In fact, from an allocation perspective, it is (nearly) the same as declaring the variable at the beginning of the function. The only difference is the scope: the variable cannot be used outside of the loop. It may even be possible that the variable is not allocated, just re-using some free slot (from other variable whose scope has ended).

With restricted and more precise scope come more accurate optimizations. But more importantly, it makes your code safer, with less states (i.e. variables) to worry about when reading other parts of the code.

This is true even outside of an if(){...} block. Typically, instead of :

    int result;
    (...)
    result = f1();
    if (result) then { (...) }
    (...)
    result = f2();
    if (result) then { (...) }

it's safer to write :

    (...)
    {
        int const result = f1();
        if (result) then { (...) }
    }
    (...)
    {
        int const result = f2();
        if (result) then { (...) }
    }

The difference may seem minor, especially on such a small example. But on a larger code base, it will help : now there is no risk to transport some result value from f1() to f2() block. Each result is strictly limited to its own scope, making its role more accurate. From a reviewer perspective, it's much nicer, since he has less long range state variables to worry about and track.

Even the compiler will help better : assuming that, in the future, after some erroneous change of code, result is not properly initialized with f2(). The second version will simply refuse to work, stating a clear error message at compile time (way better than run time). The first version will not spot anything, the result of f1() will simply be tested a second time, being confused for the result of f2().

Complementary information

The open-source tool CppCheck (a static analysis tool for C/C++ code) provides some excellent hints regarding optimal scope of variables.

In response to comment on allocation: The above rule is true in C, but might not be for some C++ classes.

For standard types and structures, the size of variable is known at compilation time. There is no such thing as "construction" in C, so the space for the variable will simply be allocated into the stack (without any initialization), when the function is called. That's why there is a "zero" cost when declaring the variable inside a loop.

However, for C++ classes, there is this constructor thing which I know much less about. I guess allocation is probably not going to be the issue, since the compiler shall be clever enough to reuse the same space, but the initialization is likely to take place at each loop iteration.

Shaneka answered 31/10, 2011 at 20:57 Comment(20)
Awesome answer. This is exactly what I was looking for, and even gave me some insight to something I didn't realize. I didn't realize that the scope remains inside the loop only. Thank you for the response!Tapping
I completely agree, but you may want to be aware that some (very) old compilers break when two loops in the same scope declare and use the same variable.Invocate
"But it will never be slower than allocating at the beginning of the function." This isn't always true. The variable will be allocated once, but it will still be constructed and destructed as many times as needed. Which in the case of the example code, is 11 times. To quote Mooing's comment "Put them close to their usage, unless profiling says otherwise."Predecessor
I was wondering about the construction and deconstruction part. Do some compilers realize that the variable will be used a certain amount of times and not deconstruct it until the loop is exited?Tapping
@Tapping : Absolutely not -- the compiler has no way of knowing if the object has meaningful side-effects in its constructor or destructor.Make
@Iron: On the other hand, when you declare the item first, you just get many calls to the assignment operator; which typically costs about the same as constructing and destroying an object.Bergen
It's not as simple as this. This answer fits C and particularily simple types where the compiler knows their size beforehand (think int, char, etc.). However, with more complex types and specially with classes that have complex constructors (e.g. a constructor that requires file or database input, complex calculations, or initialize big data structures) this can affect performance for obvious reasons, no profiling needed to notice. So for simple types yes; for complex types think first. Good practices should only be taken as a basic guide and are known to not always work in the real world.Countdown
@BillyONeal: For string and vector specifically, the assignment operator can reuse the allocated buffer each loop, which (depending on your loop) may be a huge time savings.Allegedly
Is this also valid when I'm declaring a lambda inside of a loop or should the lambda be declared outside of the loop? And does Your last little paragraph mean that for C++ it might be better to declare the variables outside the loop from a performance point of view? I really don't care about scopes in my current project, but I do indeed care a lot about performance.Yolande
The OP question was about variables, not lambda. lambda are supposed to be internal functions, so from a data flow perspective, it's completely separated from variables. Also, it should be read only, with no "initialization", so it should be static. I don't see any reason why it would make the compiler do any work. Sounds like a straight instruction pointer to me, and more likely an inlining. Caring about scope is about paying attention to code evolutions. It makes the code a lot more resistant to future changes. It's not opposed to performance, it's just orthogonal.Shaneka
It is NOT a good practice ALWAYS. What if the loop containing function is recursive? For a recursive function if it is possible to handle the logic by declaring the variable outside the function than one should not declare it inside the function. Because if it is done the function will unnecessarily make copies of the same variable up to the recursion- depth's number of times.Flavine
@MooingDuck i tested "for ( ... ) { std::string S = "a" + std::to_string(i) };" vs "std::string S {}; for ( ... ) { S = "a" + std::to_string(i) };" [in both cases an additional statement was added to "use" S (only) inside the loop, so the loop would not be optimized away]. using clang 10, any -O level, intel haswell; measured w/ xcode instruments i saw almost no diff in either short or long loops (running a minute), neither in speed nor mem allocs; the ONLY diff was the outside loop made 1 extra initial mem alloc. i tried the same thing w/ "std::vector<std::string> S" and saw the same resultsBenevento
@newbie thats because in your test you're making a new string every loop and then assigning to the variable. If you directly modify the variable instead, you should get N fewer allocations.Allegedly
@MooingDuck ah, thx, you mean using S.assign("hi") instead of S = "hi"?Benevento
@newbie: S = "hi" would be fine, because "HI" is a const char(&)[3], so does no allocations. The problem is std::to_string which allocates a new string automatically. That's the slow part. S=... just moves the content from the temporary to the variable, which is super fast.Allegedly
@newbie: Yes, since it avoids N allocations and N deallocations.Allegedly
[above answer by MooingDuck is response to the following] @MooingDuck, just to be make sure, supposing the following two having-no-effect examples dont get optimized away (btw: how to avoid this with a compiler directive?) then for(..N..){string S="a"} VS string S{};for(..N..){S="a"} the latter (define outside loop) will be O(N) faster?Benevento
@MooingDuck thx a lot. im a bit baffled by this. it means that the "define inside loop" advice is good for code compreh. but lousy for performance. since you have to consider each object and usage on an indiv. basis, at the end of the day the most consistent, easy to read & performant solution is to just define everything OUTSIDE the loop, contrary to common advice and guidelines. it seems to me a bit scandalous to have this chasm btw the recommended style (which makes sense) & actual performance (which should be no-problem via compiler optimiz.). they should be confluent/mutually supportive.Benevento
@newbie: it's not "lousy" for performance, but extracting a container outside of a loop for very careful reuse can get you some performance boosts. Note that this advice usually only applies to containers that are used in specific ways.Allegedly
@MooingDuck.ok thanks for your answers. out of respect for you and the site, i dont want to prolong this thread and i will not expect an answer from you. but i would just like to state that one of the fundamental selling points for C++ is efficiency. so if its competing with languages that are generally a mere 2x "less efficient", quirks of this sort is a real and fair concern that the language ISO standard should guarantee, in the future, will have zero-cost. (as stated by stroustrup, what you dont use you dont pay for, in this case, that, by any reasonable measure, is not true).Benevento
G
39

Generally, it's a very good practice to keep it very close.

In some cases, there will be a consideration such as performance which justifies pulling the variable out of the loop.

In your example, the program creates and destroys the string each time. Some libraries use a small string optimization (SSO), so the dynamic allocation could be avoided in some cases.

Suppose you wanted to avoid those redundant creations/allocations, you would write it as:

for (int counter = 0; counter <= 10; counter++) {
   // compiler can pull this out
   const char testing[] = "testing";
   cout << testing;
}

or you can pull the constant out:

const std::string testing = "testing";
for (int counter = 0; counter <= 10; counter++) {
   cout << testing;
}

Do most compilers realize that the variable has already been declared and just skip that portion, or does it actually create a spot for it in memory each time?

It can reuse the space the variable consumes, and it can pull invariants out of your loop. In the case of the const char array (above) - that array could be pulled out. However, the constructor and destructor must be executed at each iteration in the case of an object (such as std::string). In the case of the std::string, that 'space' includes a pointer which contains the dynamic allocation representing the characters. So this:

for (int counter = 0; counter <= 10; counter++) {
   string testing = "testing";
   cout << testing;
}

would require redundant copying in each case, and dynamic allocation and free if the variable sits above the threshold for SSO character count (and SSO is implemented by your std library).

Doing this:

string testing;
for (int counter = 0; counter <= 10; counter++) {
   testing = "testing";
   cout << testing;
}

would still require a physical copy of the characters at each iteration, but the form could result in one dynamic allocation because you assign the string and the implementation should see there is no need to resize the string's backing allocation. Of course, you wouldn't do that in this example (because multiple superior alternatives have already been demonstrated), but you might consider it when the string or vector's content varies.

So what do you do with all those options (and more)? Keep it very close as a default -- until you understand the costs well and know when you should deviate.

Godown answered 1/8, 2013 at 21:28 Comment(3)
Regarding basic datatypes like float or int, will declaring the variable inside the loop be slower than declaring that variable outside the loop as it will have to allocate a space for the variable each iteration ?Aforesaid
@Aforesaid Short answer is "No. Ignore that optimization and place it in the loop when possible for improved readability/locality. The compiler can perform that micro-optimization for you." In more detail, that is ultimately for the compiler to decide, based on what's best for the platform, optimization levels, etc. An ordinary int/float inside a loop will usually be placed on the stack. A compiler can certainly move that outside of the loop and reuse the storage if there is an optimization in doing that. For practical purposes, this would be a very very very small optimization…Godown
@Aforesaid …(cont) which you would only consider in environments/applications where every cycle counted. In that case, you might want to just consider using assembly.Godown
O
21

I didn't post to answer JeremyRR's questions (as they have already been answered); instead, I posted merely to give a suggestion.

To JeremyRR, you could do this:

{
  string someString = "testing";   

  for(int counter = 0; counter <= 10; counter++)
  {
    cout << someString;
  }

  // The variable is in scope.
}

// The variable is no longer in scope.

I don't know if you realize (I didn't when I first started programming), that brackets (as long they are in pairs) can be placed anywhere within the code, not just after "if", "for", "while", etc.

My code compiled in Microsoft Visual C++ 2010 Express, so I know it works; also, I have tried to to use the variable outside of the brackets that it was defined in and I received an error, so I know that the variable was "destroyed".

I don't know if it is bad practice to use this method, as a lot of unlabeled brackets could quickly make the code unreadable, but maybe some comments could clear things up.

Octahedron answered 8/4, 2015 at 17:11 Comment(1)
To me, this is a very legitimate answer that brings a suggestion directly linked to the question. You have my vote!Mechellemechlin
H
17

For C++ it depends on what you are doing. OK, it is stupid code but imagine

class myTimeEatingClass
{
 public:
 //constructor
      myTimeEatingClass()
      {
          sleep(2000);
          ms_usedTime+=2;
      }
      ~myTimeEatingClass()
      {
          sleep(3000);
          ms_usedTime+=3;
      }
      const unsigned int getTime() const
      {
          return  ms_usedTime;
      }
      static unsigned int ms_usedTime;
};

myTimeEatingClass::ms_CreationTime=0; 
myFunc()
{
    for (int counter = 0; counter <= 10; counter++) {

        myTimeEatingClass timeEater();
        //do something
    }
    cout << "Creating class took " << timeEater.getTime() << "seconds at all" << endl;

}
myOtherFunc()
{
    myTimeEatingClass timeEater();
    for (int counter = 0; counter <= 10; counter++) {
        //do something
    }
    cout << "Creating class took " << timeEater.getTime() << "seconds at all" << endl;

}

You will wait 55 seconds until you get the output of myFunc. Just because each loop constructor and destructor together need 5 seconds to finish.

You will need 5 seconds until you get the output of myOtherFunc.

Of course, this is a crazy example.

But it illustrates that it might become a performance issue when each loop the same construction is done when the constructor and / or destructor needs some time.

Hoboken answered 16/9, 2015 at 17:48 Comment(1)
Well, technically in the second version you will get the output in only 2 seconds, because you haven't destructed the object yet.....Lifeblood
T
2

The two snippets below generate the same assembly.

// Snippet 1
void test() { 
   int var; 
   while(1) var = 4;
}

// Snippet 2
void test() {
    while(1) int var = 4;
}

Assembly output

test():
        push    rbp
        mov     rbp, rsp
.L2:
        mov     DWORD PTR [rbp-4], 4
        jmp     .L2

Link: https://godbolt.org/z/36hsM6Pen

The default approach should be to keep declarations close to their usage unless profiling indicates performance issues or there are constructors that involve extensive computations.

Tenpins answered 30/4, 2022 at 16:41 Comment(0)
B
1

Since your second question is more concrete, I'm going to address it first, and then take up your first question with the context given by the second. I wanted to give a more evidence-based answer than what's here already.

Question #2: Do most compilers realize that the variable has already been declared and just skip that portion, or does it actually create a spot for it in memory each time?

You can answer this question for yourself by stopping your compiler before the assembler is run and looking at the asm. (Use the -S flag if your compiler has a gcc-style interface, and -masm=intel if you want the syntax style I'm using here.)

In any case, with modern compilers (gcc 10.2, clang 11.0) for x86-64, they only reload the variable on each loop pass if you disable optimizations. Consider the following C++ program—for intuitive mapping to asm, I'm keeping things mostly C-style and using an integer instead of a string, although the same principles apply in the string case:

#include <iostream>

static constexpr std::size_t LEN = 10;

void fill_arr(int a[LEN])
{
    /* *** */
    for (std::size_t i = 0; i < LEN; ++i) {
        const int t = 8;

        a[i] = t;
    }
    /* *** */
}

int main(void)
{
    int a[LEN];

    fill_arr(a);

    for (std::size_t i = 0; i < LEN; ++i) {
        std::cout << a[i] << " ";
    }

    std::cout << "\n";

    return 0;
}

We can compare this to a version with the following difference:

    /* *** */
    const int t = 8;

    for (std::size_t i = 0; i < LEN; ++i) {
        a[i] = t;
    }
    /* *** */

With optimization disabled, gcc 10.2 puts 8 on the stack on every pass of the loop for the declaration-in-loop version:

    mov QWORD PTR -8[rbp], 0
.L3:
    cmp QWORD PTR -8[rbp], 9
    ja  .L4
    mov DWORD PTR -12[rbp], 8 ;✷

whereas it only does it once for the out-of-loop version:

    mov DWORD PTR -12[rbp], 8 ;✷
    mov QWORD PTR -8[rbp], 0
.L3:
    cmp QWORD PTR -8[rbp], 9
    ja  .L4

Does this make a performance impact? I didn't see an appreciable difference in runtime between them with my CPU (Intel i7-7700K) until I pushed the number of iterations into the billions, and even then the average difference was less than 0.01s. It's only a single extra operation in the loop, after all. (For a string, the difference in in-loop operations is obviously a bit greater, but not dramatically so.)

What's more, the question is largely academic, because with an optimization level of -O1 or higher gcc outputs identical asm for both source files, as does clang. So, at least for simple cases like this, it's unlikely to make any performance impact either way. Of course, in a real-world program, you should always profile rather than make assumptions.

Question #1: Is declaring a variable inside a loop a good practice or bad practice?

As with practically every question like this, it depends. If the declaration is inside a very tight loop and you're compiling without optimizations, say for debugging purposes, it's theoretically possible that moving it outside the loop would improve performance enough to be handy during your debugging efforts. If so, it might be sensible, at least while you're debugging. And although I don't think it's likely to make any difference in an optimized build, if you do observe one, you/your pair/your team can make a judgement call as to whether it's worth it.

At the same time, you have to consider not only how the compiler reads your code, but also how it comes off to humans, yourself included. I think you'll agree that a variable declared in the smallest scope possible is easier to keep track of. If it's outside the loop, it implies that it's needed outside the loop, which is confusing if that's not actually the case. In a big codebase, little confusions like this add up over time and become fatiguing after hours of work, and can lead to silly bugs. That can be much more costly than what you reap from a slight performance improvement, depending on the use case.

Bewick answered 12/12, 2020 at 2:23 Comment(0)
M
1

Once upon a time (pre C++98); the following would break:

{
    for (int i=0; i<.; ++i) {std::string foo;}
    for (int i=0; i<.; ++i) {std::string foo;}
}

with the warning that i was already declared (foo was fine as that's scoped within the {}). This is likely the WHY people would first argue it's bad. It stopped being true a long time ago though.

If you STILL have to support such an old compiler (some people are on Borland) then the answer is yes, a case could be made to put the i out the loop, because not doing so makes it makes it "harder" for people to put multiple loops in with the same variable, though honestly the compiler will still fail, which is all you want if there's going to be a problem.

If you no longer have to support such an old compiler, variables should be kept to the smallest scope you can get them so that you not only minimise the memory usage; but also make understanding the project easier. It's a bit like asking why don't you have all your variables global. Same argument applies, but the scopes just change a bit.

Matinee answered 19/2, 2021 at 22:12 Comment(0)
S
0

It's a very good practice, as all above answer provide very good theoretical aspect of the question let me give a glimpse of code, i was trying to solve DFS over GEEKSFORGEEKS, i encounter the optimization problem...... If you try to solve the code declaring the integer outside the loop will give you Optimization Error..

stack<int> st;
st.push(s);
cout<<s<<" ";
vis[s]=1;
int flag=0;
int top=0;
while(!st.empty()){
    top = st.top();
    for(int i=0;i<g[top].size();i++){
        if(vis[g[top][i]] != 1){
            st.push(g[top][i]);
            cout<<g[top][i]<<" ";
            vis[g[top][i]]=1;
            flag=1;
            break;
        }
    }
    if(!flag){
        st.pop();
    }
}

Now put integers inside the loop this will give you correct answer...

stack<int> st;
st.push(s);
cout<<s<<" ";
vis[s]=1;
// int flag=0;
// int top=0;
while(!st.empty()){
    int top = st.top();
    int flag = 0;
    for(int i=0;i<g[top].size();i++){
        if(vis[g[top][i]] != 1){
            st.push(g[top][i]);
            cout<<g[top][i]<<" ";
            vis[g[top][i]]=1;
            flag=1;
            break;
        }
    }
    if(!flag){
        st.pop();
    }
}

this completely reflect what sir @justin was saying in 2nd comment.... try this here https://practice.geeksforgeeks.org/problems/depth-first-traversal-for-a-graph/1. just give it a shot.... you will get it.Hope this help.

Sibyls answered 7/1, 2020 at 19:37 Comment(2)
I don't think this applies to the question. Obviously, in your case above it matters. The question was dealing with the case when the variable definition could be defined elsewhere without changing the behavior of the code.Fipple
In the code you posted, the problem is not the definition but the initialization part. flag should be reinitialized at 0 each while iteration. That's a logic problem, not a definition problem.Gahan
B
-1

Chapter 4.8 Block Structure in K&R's The C Programming Language 2.Ed.:

An automatic variable declared and initialized in a block is initialized each time the block is entered.

I might have missed seeing the relevant description in the book like:

An automatic variable declared and initialized in a block is allocated only one time before the block is entered.

But a simple test can prove the assumption held:

 #include <stdio.h>                                                                                                    

 int main(int argc, char *argv[]) {                                                                                    
     for (int i = 0; i < 2; i++) {                                                                                     
         for (int j = 0; j < 2; j++) {                                                                                 
             int k;                                                                                                    
             printf("%p\n", &k);                                                                                       
         }                                                                                                             
     }                                                                                                                 
     return 0;                                                                                                         
 }                                                                                                                     
Brougham answered 9/4, 2020 at 11:37 Comment(1)
I don't understand what you're trying to say and what it has to do with the question.Capitally

© 2022 - 2024 — McMap. All rights reserved.