My C++ function has no errors, but it proceeds to not display anything

Question

I am just starting to use C++ iterators, so this code may be very bad compared to experienced programmers, but I would love all help because I am trying to get better with them. In this function, I am attempting to use a reverse iterator's position to add a comma in between each 3 letters of a number. Ex: 100000 to 100,000.

I was able to fix all errors and warnings, but the code wont output anything. It also says that I have a SIGTRAP that my program received. Please help me.

#include <iostream>
#include <vector>
#include <iterator>
#include <string>
using namespace std;
    
int main() {
    string text = "100000";
    vector<char> news{};
    for (size_t i{}; i < text.size(); i++) {
        news.push_back(text[i]);
    }
    for (vector<char>::reverse_iterator it = news.rbegin(); it != news.rend(); it = it + 2) {   
        news.insert(it.base(), ',');
    }
    string fin;
    for (char i: news)
        fin.push_back(i);
    cout << fin;
    return 0;
}

Answer 1

I normally would not use a vector this type of operation but for the sake of your learning, here are some mistakes you have made:

• During your operation, you have the vector grow, which may cause it to ask for more memory to store its content. However, as is specified in the vector::insert manual:

If after the operation the new size() is greater than old capacity() a reallocation takes place, in which case all iterators (including the end() iterator) and all references to the elements are invalidated. Otherwise, only the iterators and references before the insertion point remain valid.

What happens to iterators when you modify a container is something you have to keep in mind at all time and be careful with.
Also, generally speaking, if you can know the size of a container in advance, you should reserve the memory to avoid reallocation. I have not bothered double-checking the calculation below but I know the size I reserved is either correct or 1 byte over correct, aka accurate enough.

• If it + 1 is the past-the-end iterator of your vector, it = it + 2 is undefined behavior (UB). The code would need something to know when to stop incrementing the iterator before getting to close to rend() (as opposed to "before reaching rend()"), which is not a completely obvious condition.
  BTW, I doubt increasing the iterator by 2 would put a comma every 3 characters (not that having commas more often than desired would cause a crash nor be difficult to adjust). On the same note, you are not using the return value of vector::insert.

Correcting those mistakes results in the following code:

#include <iostream>
#include <vector>
#include <iterator>
#include <string>

int main(int argc, char* argv[])
{
    std::string text = "100000";
    auto textSize = text.size();
    //The whole operation makes sense only if we have more than 3 characters (else case is omitted here)
    if (textSize > 3) {
        std::vector<char> news{};
        //Ensure the vector will have enough storing space
        news.reserve(textSize * 4 / 3 + 1);

        //Range-based loop
        for (auto c : text) 
            news.push_back(c);

        auto it = news.rbegin(); std::advance(it, 3);
        for (size_t i = 3; i < textSize; i += 3) {
            it = std::make_reverse_iterator(news.insert(it.base(), ','));
            if (i + 3 < textSize)
                std::advance(it, 3);
        }

        std::string fin;
        for (char c : news)
            fin.push_back(c);
        std::cout << fin;
    }
    return 0;
}

---

A better way to handle this case is by avoiding using a vector completely, which would also make it more efficient. I like the approach of 2 nested loops increasing the same iterator.

For that 2nd snippet below, text is processed in left-to-right order, as appending text is the natural way strings work (not prepending, not inserting).

#include <iostream>
#include <vector>
#include <iterator>
#include <string>

int main(int argc, char* argv[])
{
    std::string text = "100000";
    auto textSize = text.size();
    //The whole operation makes sense only if we have more than 3 characters (else case is omitted here).
    if (textSize > 3) {
        std::string fin;
        //Optional but could speed up the code for long strings.
        fin.reserve(textSize * 4 / 3 + 1);

        auto it = text.begin();
        //The characters to be written before the 1st comma are handled here
        for (int i = (2 - text.size()) % 3; i < 3; ++i, ++it)
            fin.push_back(*it);

        while (it != text.end()) {
            fin.push_back(',');
            //The inner loop executes 3 times and advances the iterator
            for (int i = 0; i < 3; ++i, ++it)
                fin.push_back(*it);
        }
        std::cout << fin;
    }
    return 0;
}

Answer 2

You don't want to try to mutate a collection while iterating over it. The instant you do that you invalidate your iterators.

Rather we'll iterate over text in reverse and build up a text2 string. Then we need to reverse that string.

#include <iostream>
#include <iterator>
#include <string>
#include <algorithm>

int main() {
    std::string text = "100000";
    std::string text2;

    std::size_t i = 0;

    for (auto it = text.rbegin(); it != text.rend(); it++) {
        if (i == 3) {
            i = 0;
            text2 += ',';
        }

        text2 += *it;
        i++;
    }

    std::reverse(text2.begin(), text2.end());

    std::cout << text2 << std::endl;
}