CODEWITHSUNDEEP
c++for-loopc++11clang
yeomandev
yeomandev

Reputation: 11796

Why is my range based for loop crashing my C++ program?

I am trying to use the new C++11 range based for loops. Here's my program:

#include <iostream>
#include <string>
#include <sstream>
#include <fstream>

using namespace std;

ofstream logger("log.txt");
void log(string message)
{
    logger << message << std::endl;
    logger.flush();
}

int main( int argc, char* args[] )
{
    log("hello world");
    cout << "hello world\n";

    log("declare sort me");
    int sortMe[10];

    log("loop sortMe");
    for(int i : sortMe) {
        log("in loop " + i);
        sortMe[i] = i + 1;
    }
}

I'm using clang++ to compile. It compiles with the warning: 

clang++ -o mycpp mycpp.cpp
mycpp.cpp:24:12: warning: range-based for loop is a C++11 extension
      [-Wc++11-extensions]
        for(int i : sortMe) {
                  ^
1 warning generated.

When it runs, I get this output:

hello world
Segmentation fault (core dumped)

According to the log.txt file, the program gets to the for loop, but it never enters the for loop. What am I missing?

Upvotes: 2

Views: 3705

Answers (5)

Erik Nedwidek
Erik Nedwidek

Reputation: 6184

You can do what you want the new way, but it is a rather pointless exercise and better done with a regular for loop. The range loop returns values, not references. At least in the way you are using it.

 int count = 0;
 // Use a reference so we can update sortMe
 for (int& i : sortMe) {
    i = ++count;
 }

On looking at it, it is a little more compact than a normal for loop and strangely I prefer it. ;)

Upvotes: 0

hmjd
hmjd

Reputation: 121971

The call to log() within the for loop is incorrect. The argument is the result of pointer arithmetic, with an unknown int value being used as an offset from the base address of a string literal.

Use std::to_string() to convert an int to a std::string.

Just to mention std:iota, that can be used to set elements of a range to increasing values based of an initial value:

std::iota(std::begin(sortMe), std::end(sortMe), 1);

Upvotes: 3

templatetypedef
templatetypedef

Reputation: 372814

This loop:

for(int i : sortMe) {
    log("in loop " + i);
    sortMe[i] = i + 1;
}

Loops and returns the values stored in the sortMe array, not the indices of the sortMe array. As a result, the lookup sortMe[i] will jump to a totally random index of the array (probably way, way out of bounds), causing the segfault.

If you want to set each element equal to its position, just use a normal for loop:

for (int i = 0; i < 10; i++) {
    sortMe[i] = i + 1;
}

Also, as @hmjd noted, the call to log will not work correctly, because you are doing pointer arithmetic on a string, not doing a string concatenation.

Hope this helps!

Upvotes: 10

snooze92
snooze92

Reputation: 4238

I think that you expect the range-based loop to do something different from what it does...

If you write for (int var: array) { log(var); } then the code will be executed as many times as there are elements in the array. Each time, var will be equal to one of the elements of array. Note that I use var directly as an array element, and not array[var]!

For instance, if you have int[3] array = { 42, 69, 1337 };, the precedent for loop will log 42, 69 and 1336.

So if I simply do int[3] array;, the precedent for loop will loop the three random integers that were already in memory where the array is being stored... if instead of using var directly I was to use array[var] it would most likely crash because var would not be a valid index for array.

Solution:

Don't get confused with the difference between array elements and indexes...

  • If you want to manipulate directly the elements of the array:

    for(int element : sortMe) {
    /* Do something with the element */
}

  • If you want to use indexes, don't go for a range-based loop:

    for(int index = 0; index < 10; ++index) {
    /* Do something with the index */
}

Upvotes: 1

Joseph Mansfield
Joseph Mansfield

Reputation: 110668

You're using a range-based for loop where you should be using a standard for loop. The int i in your loop is not the index of the current element, but the value of that element. That is, if your array contained {1, 3, 3, 7}, the value of i at each iteration would be 1, then 3, then 3, then 7. Since your array is uninitialized, you have no idea what the values of i will be and you're getting undefined behaviour.

If you want the index in your for loop, use a standard for loop:

for(int i = 0; i < 10; i++) {
    log("in loop " + std::to_string(i));
    sortMe[i] = i + 1;
}

Note that to do string concatenation with +, one of your operands will need to be a std::string. Otherwise you are adding i to the pointer that points at the first character in "in loop ".

Upvotes: 5

Related Questions