[WIP] Add copy and move constructors to classes with non-default destructors

[SteveBronder]

Summary

TL;DR: Rule of 5 for our classes that's almost always the default constructors defined explicitly

When a class has only a non-default destructor defined C++ will only generate implicit copy constructors while not generating move constructors. Explicitly defining move constructors (even default defined ones) makes C++ not generate implicit copy constructors. So you need them all if you want moves essentially. I was goofing around today and did that for all the classes that have virtual or custom destructors defined. 96% of it was just adding stuff like

  gevv_vvv_vari() = default;
  explicit gevv_vvv_vari(gevv_vvv_vari&&) = default;
  gevv_vvv_vari& operator=(gevv_vvv_vari&&) = default;
  explicit gevv_vvv_vari(const gevv_vvv_vari&) = default;
  gevv_vvv_vari& operator=(const gevv_vvv_vari&) = default;

But for things like vari there are particulars that I'll discuss below. The functions that this touch are

Memory Stuff

1. stack_alloc
   • Or should the copy and move constructors be deleted for stack_alloc?
2. vari
   • I'm not sure if I implemented these correctly, mostly surrounding var_stack part of the chainableAllocator.
3. gevv_vvv_vari
4. mdivide_left_ldlt_alloc

(3) and (4) are pretty standard imo and I don't see any weirds coming from them, tbh we could probably just delete the empty destructor since eod it is still calling the destructors of it's members

ODE Solvers

5. cvodes_ode_data
6. idas_forward_system
7. idas_system
8. kinsol_system_data

@yizhang-yiz I know you folks have to interact with a C API for sundials, does it make sense to move those objects in the class with the default move constructor? We can remove those if you don't want them.

I also deleted the move constructor and assignment operator for AutodiffStackStorage and AutodiffStackSingleton. They are implicitly not created but since the purpose of that class is to be a singleton I think it's better to be explicit.

Tests

Part of the WIP is making sure these moves make sense

Side Effects

Usually I say something like "I hope not!" here but this time, Yes! Sort of. If the compiler has an opportunity to use a move for these classes, it can! 99% of the time this is inconsequential since we don't use std::forward or std::move very often and we also use const& for the input of our signatures. But say we had a function like below.

template <typename T, require_var<T>...>
inline var atan(T&& a) { return {new internal::atan_vari(a.vi_)}; }

Well now we could call that function from a higher scope like atan(std::forward<Var>(my_var)). Or ala eigen

A = B.unaryExpr([](auto&& x) { return atan(x) })

Though I'm not sure if the eigen one is as interesting

But thats the jist, yes there are side-effects. We'll be able to start thinking about moving memory down just like how RVO sends it up.

I'm Not 100% on Vari's assignments

It looks like this, the members are plain old data so it's fine to just copy them

  vari& operator=(vari&& x) noexcept {
    // (1) Val is const and cannot be assigned to
    //this->val_ = x.val_;
    this->adj_ = x.adj_;
    if (x.stacked_ && !this->stacked_) { // (2)
      ChainableStack::instance_->var_stack_.emplace_back(std::move(this));
    } else if (!x.stacked_ && this->stacked_) {
      ChainableStack::instance_->var_nochain_stack_.emplace_back(std::move(this));
    }
    this->stacked_ = x.stacked_;
    return *this;
  };

1. (this also applies to the copy assignment) Conceptually, does it make sense to do an assignment to another vari? The val_ member is const so we can't change it, but that just means we can only change adj_. Removing the const on val_ feels like a not small thing to do so I wanted to check whether there's another solution besides removing that const which feels like a possible issue.

2. Will there ever be a circumstance where a vari from the var_stack_ will be assigned to a vari from the var_nochain_stack_ ala below? Also the std::move(this) is kind of dumb looking, but in my brain, if something sits in the var_stack_that is assigned from a thing on thevar_nochain_stack_, shouldn't it be moved to var_nochain_stack_`? tbh I'm pretty not sure how to operate on this one.

Checklist

• Math issue Update internals to use more modern c++ #1308

• Copyright holder: Steve Bronder

  The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
  - Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
  - Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)

• the basic tests are passing

  • unit tests pass (to run, use: ./runTests.py test/unit)
  • header checks pass, (make test-headers)
  • docs build, (make doxygen)
  • code passes the built in C++ standards checks (make cpplint)

• the code is written in idiomatic C++ and changes are documented in the doxygen

• the new changes are tested

[stan-buildbot]

(stat_comp_benchmarks/benchmarks/gp_pois_regr/gp_pois_regr.stan, 0.99)
(stat_comp_benchmarks/benchmarks/low_dim_corr_gauss/low_dim_corr_gauss.stan, 0.98)
(stat_comp_benchmarks/benchmarks/irt_2pl/irt_2pl.stan, 0.99)
(stat_comp_benchmarks/benchmarks/pkpd/one_comp_mm_elim_abs.stan, 1.0)
(stat_comp_benchmarks/benchmarks/eight_schools/eight_schools.stan, 1.0)
(stat_comp_benchmarks/benchmarks/gp_regr/gp_regr.stan, 1.0)
(stat_comp_benchmarks/benchmarks/arK/arK.stan, 0.98)
(performance.compilation, 1.02)
(stat_comp_benchmarks/benchmarks/low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan, 0.97)
(stat_comp_benchmarks/benchmarks/low_dim_gauss_mix/low_dim_gauss_mix.stan, 0.98)
(stat_comp_benchmarks/benchmarks/sir/sir.stan, 0.93)
(stat_comp_benchmarks/benchmarks/pkpd/sim_one_comp_mm_elim_abs.stan, 1.02)
(stat_comp_benchmarks/benchmarks/garch/garch.stan, 0.82)
(stat_comp_benchmarks/benchmarks/gp_regr/gen_gp_data.stan, 1.0)
(stat_comp_benchmarks/benchmarks/arma/arma.stan, 1.0)
Result: 0.97860879209
Commit hash: 73b2f31

[bob-carpenter]

... I'm Not 100% on Vari's assignments It looks like this vari& operator=(vari&& x) noexcept { // (1) Val is const and cannot be assigned to //this->val_ = x.val_; • (this also applies to the copy assignment) Conceptually, does it make sense to do an assignment to another vari? The val_ member is const so we can't change it, but that just means we can only change adj_. Removing the const on val_ feels like a not small thing to do so I wanted to check whether there's another solution besides removing that const which feels like a possible issue. • Will there ever be a circumstance where a vari from the var_stack_ will be assigned to a vari from the var_nochain_stack_ ala below? Also the std::move(this) is kind of dumb looking, but in my brain, if something sits in the var_stack_that is assigned from a thing on thevar_nochain_stack_, shouldn't it be moved to var_nochain_stack_`? tbh I'm pretty not sure how to operate on this one.

This is great. We should be moving toward move semantics where possible. Is there a motivation for assigning vari? I thought we only needed to assign vari* inside a var---once a vari gets created, it's constant other than for the adjoint value during the reverse pass of autodiff. So if you need the signature, can it just throw?

[SteveBronder]

Is there a motivation for assigning vari?

After looking at it idt so, since its only used in var at most the assign for var would copy the pointer to the vari over and not the vari itself. I'll delete their assign operators and check if that's fine

[syclik]

@SteveBronder, closing for now since it's still WIP. The ideas here are awesome and I'm all for it.

[SteveBronder]

yeah apologies there are like 2-4 PRs that got bungalowed for various reasons. It's fine and good to close this for now