Replacing a local object causes warnings (deprecated-copy)

[ksh02]

Hi C++ forum, I have an issue regarding this snippet

StringSeq result = StringSeq(seq);
for (Variant* var : vars)
{
  result = var->applyTo(result, offset);
}
return result;

I'm trying to apply these "variants" in succession. The applyTo function returns a new StringSeq which is, as the name implies, a variant of the one passed as an argument. Compiling this gives the following warning :

variant.cc:103:67: warning: implicitly-declared ‘dnaseq::StringSeq& dnaseq::StringSeq::operator=(const dnaseq::StringSeq&)’ is deprecated [-Wdeprecated-copy]
  103 |       result = var->applyTo(result, offset);
      |                                                                   ^
In file included from variant.h:17,
                 from variant.cc:8:
sequence.h:25:7: note: because ‘dnaseq::StringSeq’ has user-provided ‘dnaseq::StringSeq::StringSeq(const dnaseq::StringSeq&)’
   25 |       StringSeq(StringSeq const &strSeq);
      |       ^~~~~~~~~

How can I not "copy" the object but replace the one the local result variable points to? I've tried making it a pointer but its not possible to get the pointer like &(var->applyTo(result, offset)) as it is a rvalue. I don't know what that means. It works when ignoring the warning. Also if I'm approaching this badly and the idea is flawed I'm all ears.

[newbieg]

It's hard to do anything without seeing the whole project, but I think the issue is the following;
https://stackoverflow.com/questions/51863588/warning-definition-of-implicit-copy-constructor-is-deprecated

"The implicit definition of a copy constructor as defaulted is deprecated if the class has a user-declared copy assignment operator or a user-declared destructor. The implicit definition of a copy assignment operator as defaulted is deprecated if the class has a user-declared copy constructor or a user-declared destructor."

... So it seems that if you declare a copy constructor or a destructor, then you need to also define your own copy assignment operator (and visa-versa). Basically if you declare one then you should probably define all three.

[mbozzi]

Depending on the definition of StringSeq it could be fine to silence or ignore the warning here. If you control the StringSeq you could also add a copy-assignment operator as suggested.

The warning is an observation that a user-provided copy constructor is likely to be inconsistent with an implicitly-declared ("compiler-provided") copy assignment operator. Inconsistency means that the copy-constructor probably makes a "deep copy", where the copy-assignment operator merely makes a "shallow copy".

This is the "Rule of Three", a particularly useful guideline which says "If a class requires a user-defined copy-assignment operator, copy constructor, or destructor, it is likely to require all of them".
https://en.cppreference.com/w/cpp/language/rule_of_three

my_class declared below exhibits the inconsistency that the compiler is warning you about:

#include <iostream>

class my_class
{ 
  int* px;
  
public:
  my_class(my_class const& that) 
    : px{ new int{*that.px} } 
  {}
  
  // should also provide a copy-assignment operator as follows:
  // the absence of the following line is the cause of the warning:
  //   my_class& operator=(my_class const& rhs) { *px = *rhs.px; return *this; }
  // the compiler-provided version of this function has different semantics,
  // it would perform member-wise assignment as-in the following implementation
  //   my_class& operator=(my_class const& rhs) { px = rhs.px; return *this; }
  
  // for completeness's sake a destructor should be provided to ensure that 
  // the memory obtained with new is subsequently and predictably deleted in 
  // every circumstance:
  ~my_class() { delete px; }
  
public:
  explicit my_class(int n = 0) : px{ new int{n} } {}
  int get() const { return *px; } 
  void set(int n) { *px = n; }
};

int main()
{
  my_class a { 1 }; 
  my_class b { a };
  my_class c; 
  c = a;
  
  std::cout << a.get() << ' ' << b.get() << ' ' << c.get() << '\n'; // 1 1 1 
  b.set(2); // modifications to b are indepedndent of both a and c
  std::cout << a.get() << ' ' << b.get() << ' ' << c.get() << '\n'; // 1 2 1
  a.set(3); // modifications to a are reflected in both a and c
  std::cout << a.get() << ' ' << b.get() << ' ' << c.get() << '\n'; // 3 2 3
}

Ordinarily we would expect the last line of code to print 3 2 1, because after c = a object c would conventionally be independent from a.
The program isn't wrong but just "surprising".

How can I not "copy" the object but replace the one the local result variable points to?

To decide how to proceed we would need to fully understand result = var->applyTo(result, offset) so we could adjust its meaning if necessary. In particular we could evaluate adding a move constructor and move assignment operator to StringSeq.

There might be little to change within the code you posted.

[ksh02]

StringSeq and Variant are both classes I control.
I've tried overloading the assignment operator and that does indeed fix the warning.

StringSeq& StringSeq::operator=(StringSeq const& other)
{
  data = other.data; return *this;
}

Variant is an abstract base class that calls the applyTo function of its derived classes.
Substitution is one of those. This function used to cause the same warning as the original snippet.

StringSeq Variant::applyTo(StringSeq& seq, int const offset)
{
  // the offset parameter has not been implemented yet
  const std::type_info& ti = typeid(*this);
  StringSeq result;
    
  if (ti == typeid(Substitution))
    result = static_cast<Substitution const*>(this)->applyTo(seq);
    
  return result;
}

StringSeq Substitution::applyTo(StringSeq& seq) const
{
  std::string str = seq.to_string();
  str[start] = nwChar; // replace a char
  return StringSeq(str);
}

I don't want to use function overriding because some of the other types of sequences I have to implement this for (there's more than just StringSeq) are template classes. Also there's some logic to do with the offset parameter that could be done surrounding of the subclass implementation of applyTo.

Substitution::applyTo already generates a new object so I think this is solved.
Thanks for your help!

[seeplus]

StringSeq& StringSeq::operator=(StringSeq const& other)
{
  data = other.data; return *this;
}

If .data is a pointer to memory, this is almost certainly incorrect as this is a shallow copy rather than the probably required deep copy.

Also, if you provide a copy constructor as well as providing operator=, you probably should also provide a move constructor and a move assignment (which can be combined with the copy one if copy/swap is used).