fix: make factory functions use std::move instead of std::forward

[andre-nguyen]

@nathanhhughes

Unfortunately, my previous PR #44, while adding support for move only types, actually ended up introducing a bug where l-values end up being casted to r-value references. This casting then makes the factory function lookup fail. The following unit test looks like it should work, but doesn't actually pass after #44. The reason the PR unit tests pass is because all the factory unit tests were written with r-values and no l-values.

class Base {
 public:
  explicit Base(int i) : i_(i) {}
  virtual ~Base() = default;
  const int i_;
  virtual std::string name() const = 0;
};

class DerivedB : public Base {
 public:
  explicit DerivedB(int i) : Base(i) {}
  std::string name() const override { return "DerivedB"; }
  inline static const auto registration_ = config::Registration<Base, DerivedB, int>("DerivedB");
};

TEST(Factory, lvalueCreate) {
  int i = 1;
  auto base = create<Base>("DerivedB", i);
  EXPECT_TRUE(base);
  EXPECT_EQ(base->name(), "DerivedB");
}

With the error being:

unknown file: Failure
C++ exception with description "Invalid module map for type 'DerivedB' 
and info BaseT='config::test::Base' and ConstructorArguments={'int'}" thrown in the test body.

My understanding here is that the function signature with the forwarding reference && below causes the incorrect type to be deduced.

template <typename BaseT, typename... ConstructorArguments>
std::unique_ptr<BaseT> create(const std::string& type, ConstructorArguments&&... args)

The type of args ends up being deduced as int& through reference collapsing rules, which then ends up causing a factory method lookup failure later. As proof of this theory, we can actually make the test pass by casting to i to int. Or explicitly specifying ConstructorArguments and std::move'ing i to allow the r-value reference to bind to i.

TEST(Factory, lvalueCreate) {
  int i = 1;
  auto base = create<Base>("DerivedB", static_cast<int>(i));
  // This also works: create<Base, int>("DerivedB", std::move(i));
  EXPECT_TRUE(base);
  EXPECT_EQ(base->name(), "DerivedB");
}

The fix

This PR fixes the issue by first reverting the previous commit, and then adding std::move instead of std::forward everywhere the args are used. Several unit tests are added to ensure r-value, r-value reference, l-value, const l-value and move only types work correctly.

[Schmluk]

Great catch! Looks good and thanks for adding the utests!

[nathanhhughes]

@andre-nguyen thanks for this! I had seen it on my side when I tried to run downstream code, but like your fix better than what mine was shaping up to be 😄