Array manipulation: erasing members while looping through the array

[Acherrum]

So, I've been working on a wrapper for RapidJSON for the company I work at and so far, everyone's been really enjoying it.

Now I've created some tool using that wrapper to quickly update our 15k+ JSON models to a new format. Which mostly works fine, except that I seem to have encountered a bug where RapidJSON allocators seem to lose track when iterating through an array, that is changing size during the iteration.

Here's some test code to illustrate the issue:

static const std::string cJSONArray =
    R"raw({
    "randomIds": ["L1_02","L1_03","L1_04","L1_05","L1_06","L1_07","L1_08",
    "L1_09","L1_10","L1_11","L1_12","L1_13","L1_14","L1_15"]
    })raw";

void Print(const rapidjson::Value &aValue) {
  rapidjson::StringBuffer lSB;
  rapidjson::Writer<rapidjson::StringBuffer> lWriter(lSB);

  aValue.Accept(lWriter);

  std::cout << lSB.GetString() << std::endl;
}

int main() {

  rapidjson::Document lDocument;
  lDocument.Parse(cJSONArray);

  // loop through the array of ids; use Size() to get the new size after manipulation
  for (unsigned int lIndex = 0; lIndex < lDocument["randomIds"].Size(); ++lIndex)
  {
    std::string lTemp = lDocument["randomIds"][lIndex].GetString();
    // remove a specific element from the array, and add it to the main document
    if (lTemp == "L1_03")
    {
      // first we add the copied text
      lDocument.AddMember("test", lTemp, lDocument.GetAllocator());
      bool lDone = false;
      unsigned int lIndexCounter = 0;
      // now iterate through the array again, to find the correct iterator so we can call Erase()
      for (rapidjson::Value::ConstValueIterator lItr = lDocument["randomIds"].Begin();
           (lItr != lDocument["randomIds"].End()) && (!lDone); ++lItr) {
        if (lIndexCounter == lIndex) {
          // Erase and then update the main index counter
          lDocument["randomIds"].Erase(lItr);
          --lIndex;
          lDone = true;
        }
        ++lIndexCounter;
      }
    }
    else
    {
      lTemp = "Test" + std::to_string(lIndex);
      lDocument["randomIds"][lIndex].SetString(rapidjson::StringRef(lTemp), lTemp.length());
    }
    Print(lDocument);
  }
}

The output is this:

{"randomIds":["Test0","L1_03","L1_04","L1_05","L1_06","L1_07","L1_08","L1_09","L1_10","L1_11","L1_12","L1_13","L1_14","L1_15"]}
{"randomIds":["L1_03","L1_04","L1_05","L1_06","L1_07","L1_08","L1_09","L1_10","L1_11","L1_12","L1_13","L1_14","L1_15"],"test":"L1_03"}
{"randomIds":["L1_04","Test1","L1_05","L1_06","L1_07","L1_08","L1_09","L1_10","L1_11","L1_12","L1_13","L1_14","L1_15"],"test":"L1_03"}
{"randomIds":["2\u0000_04","L1_05","Test2","L1_06","L1_07","L1_08","L1_09","L1_10","L1_11","L1_12","L1_13","L1_14","L1_15"],"test":"L1_03"}
{"randomIds":["Test3","3\u0000_05","L1_06","Test3","L1_07","L1_08","L1_09","L1_10","L1_11","L1_12","L1_13","L1_14","L1_15"],"test":"L1_03"}
{"randomIds":["L1_07","Test4","4\u0000_06","L1_07","Test4","L1_08","L1_09","L1_10","L1_11","L1_12","L1_13","L1_14","L1_15"],"test":"L1_03"}
{"randomIds":["5\u0000_07","L1_08","Test5","5\u0000_07","L1_08","Test5","L1_09","L1_10","L1_11","L1_12","L1_13","L1_14","L1_15"],"test":"L1_03"}
{"randomIds":["Test6","6\u0000_08","L1_09","Test6","6\u0000_08","L1_09","Test6","L1_10","L1_11","L1_12","L1_13","L1_14","L1_15"],"test":"L1_03"}
{"randomIds":["L1_10","Test7","7\u0000_09","L1_10","Test7","7\u0000_09","L1_10","Test7","L1_11","L1_12","L1_13","L1_14","L1_15"],"test":"L1_03"}
{"randomIds":["8\u0000_10","L1_11","Test8","8\u0000_10","L1_11","Test8","8\u0000_10","L1_11","Test8","L1_12","L1_13","L1_14","L1_15"],"test":"L1_03"}
{"randomIds":["Test9","9\u0000_11","L1_12","Test9","9\u0000_11","L1_12","Test9","9\u0000_11","L1_12","Test9","L1_13","L1_14","L1_15"],"test":"L1_03"}
{"randomIds":["L1_13","Test1","10\u000012","L1_13","Test1","10\u000012","L1_13","Test1","10\u000012","L1_13","Test10","L1_14","L1_15"],"test":"L1_03"}
{"randomIds":["11\u000013","L1_14","Test1","11\u000013","L1_14","Test1","11\u000013","L1_14","Test1","11\u000013","L1_14\u0000","Test11","L1_15"],"test":"L1_03"}
{"randomIds":["Test1","12\u000014","L1_15","Test1","12\u000014","L1_15","Test1","12\u000014","L1_15","Test1","12\u000014\u0000","L1_15\u0000","Test12"],"test":"L1_03"}

As you can clearly see, something goes terribly wrong after the first iteration ...
The first time, Test0 is correctly written to the first element. However, when the 2nd element gets copied and erased, the entire array starts falling apart.

[miloyip]

Similiar to std::vector::erase(), you need to use the returned iterator after erasing an element:

    //! Remove an element of array by iterator.
    /*!
        \param pos iterator to the element to remove
        \pre IsArray() == true && \ref Begin() <= \c pos < \ref End()
        \return Iterator following the removed element. If the iterator pos refers to the last element, the End() iterator is returned.
        \note Linear time complexity.
    */
    ValueIterator Erase(ConstValueIterator pos) {
        // ...
    }

[Acherrum]

Thanks. Did not realise the Erase() worked the same as with a vector 📄

Update:
It did not fix the issue I'm having though; I'm actually getting the exact same result as in my original post. I'll continue testing/trying different approaches.

Update #2:
Switched to pointers in the last else-case, and that fixed the writing to the array elements.
This code works fine now. Thanks.

[pah]

It did not fix the issue I'm having though; I'm actually getting the exact same result as in my original post.

You increment the iterator twice in case of a successful Erase, once because Erase's return value points to the next element already and you increment lItr unconditionally in the loop. You would need something like the following:

      for (rapidjson::Value::ConstValueIterator lItr = lDocument["randomIds"].Begin();
           (lItr != lDocument["randomIds"].End()) && (!lDone); /* ++lItr */ ) //<-- don't increment lItr here
      {
        if (lIndexCounter == lIndex) {
          // Erase and then update the main index counter
          lItr = lDocument["randomIds"].Erase(lItr);
          --lIndex;
          lDone = true;
        } else {
          ++lItr; // <-- increment, if not erased
        }
        ++lIndexCounter;
      }

[Acherrum]

Thanks, @pah.
I did not make that mistake though ;) I rewrote the for-loop to a while-loop and used that same if/else construction you showed there.
However, as I mentioned in my 2nd update, it did not work when I used Value&.
When I rewrote it to use Value* instead of the Value& (rapidjson::Pointer vs lDocument[]), the code worked perfectly.

I still noticed an issue when using erase and set, using only Value* and the nesting was multiple levels deep. But I fixed that by specifically using the RootDocument when erasing.

[pah]

I did not make that mistake though ;)

Apologies for implying that you might have overlooked this. :-)

I still noticed an issue when using erase and set, using only Value* and the nesting was multiple levels deep.

Hard to tell, what might have gone wrong.

There is another potential issue in the original code: The use of StringRef on a changing string. You change the contents of lTemp several times in the loop, but still store the same pointer into the document at different indices?