randv (and related classes) don't use RAII

[AnthonyVH]

When a crave::randv object is constructed with a non-nullptr value for the parent pointer, it registers itself with said parent (through randv_base::add_base_child). However, upon destruction it doesn't deregister itself from the parent. This causes dangling pointers and the undefined behavior that comes with it when rand_base::next() is called on the parent (so far I've seen SIGSERV and "abstract virtual called").

Furthermore, since there's copy constructors defined for the randv_base and randv types, move construction and assignment are both implicitly disabled. Just "thinking out loud", it seems to me that when a parent class is moved, it should be able to call some sort of "move_parent()" on the randv child objects. Such move_parent functionality would be the equivalent of remove_base_child (which currently doesn't exist) followed by add_base_child. This move_parent could then also be called by a user's custom objects where appropriate.

Similar things can most likely be said about rand_obj and its add_obj_child function (although I didn't check this thoroughly yet).

Below is a minimalistic example that triggers UB through derefencing pointers to destructed objects. I know crave::rand_vec exists, but I don't see why this example using std::vector shouldn't be able to work if the above issues are solved.

#include <crave/ConstrainedRandom.hpp>

#include <cstdint>
#include <iostream>
#include <vector>

struct custom_item : public crave::rand_obj {
  using base_type = crave::rand_obj;

  custom_item (crave::rand_obj * parent = nullptr)
    : base_type(parent), vectorSize_(this)
  {
    constraint(0 < vectorSize_() && vectorSize_() <= 10);
  }

  virtual bool next () override {
    bool const result = base_type::next();
    post_randomize();
    return result;
  }

  virtual void post_randomize () {
    items_.clear();
    items_.reserve(vectorSize_);
    for (std::size_t count = 0; count < vectorSize_; ++count) {
      items_.emplace_back(this);
      items_.back().next();
    }
  }

  crave::randv<std::size_t> vectorSize_;
  std::vector<crave::randv<int>> items_ = {};
};

std::ostream & operator<< (std::ostream & os, custom_item const & obj) {
  os << "elements [" << obj.vectorSize_ << "]: ";
  for (auto const & entry : obj.items_) { os << entry << ' '; }
  return os;
}

int main () {
  crave::init("crave.cfg");
  crave::set_global_seed(1234567890); // Raises SIGSERV.
  //crave::set_global_seed(2234567890); // Raises abstract virtual called.

  custom_item anItem;

  for (int i = 0; i < 5; i++) {
    CHECK(anItem.next());
    std::cout << anItem << '\n';
  }
}

[hoangmle]

We did not intend to support manipulation of the randv/rand_obj object tree from user code, but it would have been useful to detach objects on their destruction to prevent such unwanted UBs. May I ask you to continue your evaluation on the experimental API (which does support some form of RAII if I remember correctly) since the 2.0 one is going to be deprecated soon?

As for your example, I would just use a vector<int> and a single randv<int> to fill the vector with generated values in a loop. This also helps to avoid constructing/deleting multiple randv at run-time.

[AnthonyVH]

Sure thing, I'll try any new experiments against the experimental API.

Using a single randv<int> to populate the vector would definitely be more efficient in this case.