ParsedFunction: change to vectors of unique_ptrs

[jwpeterson]

This is similar to the change made in 783091b for
ParsedFEMFunction. For a class to be pushed-back onto a vector, it
must be MoveInsertable, which FunctionParserADBase is not.

Refs #2710

[jwpeterson]

@roystgnr if you could test this with GRINS again that would be good. It's looking like this and 783091b will indeed need to be backported to the 1.4.x and 1.5.x branches.

[roystgnr]

Glad I tested it. This one's actually failing to compile with GRINS for me.

When trying to initialize a vector of ParsedFunction objects, gcc 9.3.0-10ubuntu2 gives me:

/usr/include/c++/9/bits/stl_uninitialized.h: In instantiation of ‘_ForwardIterator std::uninitialized_fill_n(_ForwardIterator, _Size, const _Tp&) [with _ForwardIterator = libMesh::ParsedFunction<double, libMesh::VectorValue<double> >*; _Size = long unsigned int; _Tp = libMesh::ParsedFunction<double, libMesh::VectorValue<double> >]’:
/usr/include/c++/9/bits/stl_uninitialized.h:384:39:   required from ‘_ForwardIterator std::__uninitialized_fill_n_a(_ForwardIterator, _Size, const _Tp&, std::allocator<_Tp2>&) [with _ForwardIterator = libMesh::ParsedFunction<double, libMesh::VectorValue<double> >*; _Size = long unsigned int; _Tp = libMesh::ParsedFunction<double, libMesh::VectorValue<double> >; _Tp2 = libMesh::ParsedFunction<double, libMesh::VectorValue<double> >]’
/usr/include/c++/9/bits/stl_vector.h:1593:33:   required from ‘void std::vector<_Tp, _Alloc>::_M_fill_initialize(std::vector<_Tp, _Alloc>::size_type, const value_type&) [with _Tp = libMesh::ParsedFunction<double, libMesh::VectorValue<double> >; _Alloc = std::allocator<libMesh::ParsedFunction<double, libMesh::VectorValue<double> > >; std::vector<_Tp, _Alloc>::size_type = long unsigned int; std::vector<_Tp, _Alloc>::value_type = libMesh::ParsedFunction<double, libMesh::VectorValue<double> >]’
/usr/include/c++/9/bits/stl_vector.h:522:9:   required from ‘std::vector<_Tp, _Alloc>::vector(std::vector<_Tp, _Alloc>::size_type, const value_type&, const allocator_type&) [with _Tp = libMesh::ParsedFunction<double, libMesh::VectorValue<double> >; _Alloc = std::allocator<libMesh::ParsedFunction<double, libMesh::VectorValue<double> > >; std::vector<_Tp, _Alloc>::size_type = long unsigned int; std::vector<_Tp, _Alloc>::value_type = libMesh::ParsedFunction<double, libMesh::VectorValue<double> >; std::vector<_Tp, _Alloc>::allocator_type = std::allocator<libMesh::ParsedFunction<double, libMesh::VectorValue<double> > >]’
../../src/physics/src/convection_diffusion.C:50:168:   required from here
/usr/include/c++/9/bits/stl_uninitialized.h:265:63: error: static assertion failed: result type must be constructible from input type
  265 |       static_assert(is_constructible<_ValueType, const _Tp&>::value,
      |                                                               ^~~~~

[roystgnr]

It's possible that the ParsedFEMFunction fix had the same problem - grep 'vector<libMesh::Parsed' src/*/*/*/* in GRINS shows that vector of ParsedFunction but no vectors of ParsedFEMFunction.

[jwpeterson]

Hmm... the line numbers are a bit off, but I guess it's referring to:

_v(3,libMesh::ParsedFunction<libMesh::Number>("0.0") ),

I don't really see how my change would cause any issues with this line as it's calling the ParsedFunction constructor that takes a std::string... and I didn't modify that one. I guess one possibility is that it was relying on the copy constructor? I deleted that since it could no longer be defaulted once I added unique_ptrs to the class, but I guess we could add a hand-coded copy constructor instead?

[roystgnr]

I guess one possibility is that it was relying on the copy constructor?

That's what it looks like to me.

I guess we could add a hand-coded copy constructor instead?

Yeah, I think that's important enough to support. Unless you've already started on it, I'll take a crack at it?

[jwpeterson]

Yeah, I think that's important enough to support. Unless you've already started on it, I'll take a crack at it?

Please go ahead and just push to this branch if you don't mind. I'll work on other stuff instead.

[roystgnr]

I'd appreciate a double-check on the code, but this is passing tests in GRINS now, and I've added a couple lines to the unit tests to make sure "we can push them into vectors and get out the expected results still" is covered in CI here from now on.

[jwpeterson]

Looks OK to me... I think the other option is to implement the copy constructor and then implement operator= using a std::swap() similar to what we did in dirichlet_boundary.C (ca338c6). I don't know if there was a big reason to prefer one over the other other than copy-construct-then-swap is theoretically faster than default-constuct-then-assign.

[roystgnr]

I don't know if there was a big reason to prefer one over the other other than copy-construct-then-swap is theoretically faster than default-constuct-then-assign.

I should have remembered that. This copying should never be happening in an inner loop, though, so as long as my shame isn't going to show up in anyone's timings I'd say let's just merge as-is.

[jwpeterson]

I remembered the other reason to use the copy-and-swap idiom, which is otherwise you need to remember to guard against self-assignment, which your current implementation does not do... I'll throw a version up there and we can decide whether we like it...

[roystgnr]

Ooh, good catch. Self-assignment in my version would have just been an efficiency issue, not a memory leak like in that StackOverflow example, but your version is still much better.

[roystgnr]

And your version is still happy with my unit tests and with GRINS. Good to go.