C++ Vector subscript out of range (but it's not)

Question

I'm making a bottom-top rgb pixel array to top-bottom. I've checked the value of something and it gives me the expected output. No value is greater than obj.size() and no value is less than 0, I don't know what's up :/

std::vector<std::string> BMP_READER::TopBottom(std::vector<std::string> obj)
{
    if (y_height <= 1) { return obj; } // nothing to reverse if its only one row 

    std::vector<std::string> new_v;

    for (int h = 0; h < y_height; h++)
    {
        for (int i = x_width; i >= 1; i--)
        {
            int something = (obj.size() - i) - (x_width*h); // error
            std::string val = obj[something];

            new_v.push_back(val);
        }
    }

    return new_v;
}

"No value is greater than `obj.size()`" That may not be enough: it must be strictly less than `obj.size()`, i.e. values equal to `obj.size()` are not allowed. — Sergey Kalinichenko, Dec 09 '13 at 04:14
Even when I don't index by anything, the error is there. I just tried removing the for loop that checks the vector the function returns. It still would crash when I open the program — Kevin doesmath, Dec 09 '13 at 04:15
Since you're flipping both left-to-right and top-to-bottom, why isn't this just a `std::reverse`, or a `std::copy` from `rbegin` to `rend`? — Joe Z, Dec 09 '13 at 04:17
The `- (x_width*h)` smells wrong. The use of variables that tell you the size of a `std::vector` smells wrong (it knows its size). Not using **`std::reverse`** smells wrong. — Cheers and hth. - Alf, Dec 09 '13 at 04:18
@KevinKevinz: weird. If you take out the indexing by something and the push_back it should not crash. — carlosdc, Dec 09 '13 at 04:19
by the way, it's a good idea to reserve ALL UPPERCASE names for macros. — Cheers and hth. - Alf, Dec 09 '13 at 04:22
Thanks for all your replies, I was just curious what was wrong with my function. I'll use std::reverse. — Kevin doesmath, Dec 09 '13 at 14:44

Joe Z · Answer 1 · 2013-12-09T04:29:54.480

1

You should be able to replace your entire function with:

#include <algorithm>

std::vector<std::string> BMP_READER::TopBottom(std::vector<std::string> obj)
{
    std::reverse(obj.begin(), obj.end());
    return obj;
}

Note that this will put the lower-left corner in the upper-right corner. This line in your code suggests you only want to mirror top-to-bottom:

if (y_height <= 1) { return obj; } // nothing to reverse if its only one row

If you want to swap rows, but keep the pixels left-to-right within each row, then the following ought to do:

#include <algorithm>

std::vector<std::string> BMP_READER::TopBottom(std::vector<std::string> obj)
{
    int top_row = 0, bot_row = y_height - 1;

    while (top_row < bot_row)
    {
        std::swap_ranges( obj.begin() + top_row * x_width,
                          obj.begin() + top_row * (x_width + 1),
                          obj.begin() + bot_row * x_width );

        top_row++;
        bot_row--;
    }

    return obj;
}

edited Dec 09 '13 at 04:29

answered Dec 09 '13 at 04:24

Joe Z

17,413
3
28
39

As I recall it, to get move semantics for the return in this case `return move( obj );` will help. – Cheers and hth. - Alf Dec 09 '13 at 04:27
@Cheersandhth.-Alf More than likely, it will be elided. – Zac Howland Dec 09 '13 at 04:28
@ZacHowland; yeah, but it's ungood to rely on possible RVO when one can guarantee things with almost no effort. – Cheers and hth. - Alf Dec 09 '13 at 04:29
@Cheersandhth.-Alf Section 12.8/32 makes `std::move(obj)` superfluous. It is not simply "relying on possible RVO", it is actually written in the standard. See Steve's answer [here](http://stackoverflow.com/a/14856553/529761) for more details. – Zac Howland Dec 09 '13 at 15:25

score 0 · Answer 2 · answered Dec 09 '13 at 04:27

If you are required to write your own version of reverse:

std::vector<std::string> BMP_READER::TopBottom(std::vector<std::string> obj)
{
    for (std::size_t i = 0; i < obj.size() / 2; ++i)
    {
        std::swap(obj[i], obj[obj.size() - i - 1]);
    }
    return obj;
}

Which is a single for-loop that only iterates through half of the elements.

Otherwise, use std::reverse.

C++ Vector subscript out of range (but it's not)

2 Answers2