Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link
  • One small error: Mark the move constructor as noexcept, otherwise there are situations where it won’t be used there are situations where it won’t be used. The linked case is different since the class has a copy-constructor, yet I can imagine that there are still situations where it matters, especially since your copycon isn’t deleted, just private and undefined.

  • Why is the destructor virtual? If the example is supposed to be minimal then this might be distracting.

  • In generate_it:

     return std::move(Moveable(foo));
    

    The std::move here is redundant, since you are returning a temporary. What’s more, the explicit constructor call is redundant too, since the constructor isn’t marked as explicit. Just return foo; will do.

  • The find_if call could be replaced by vanilla find, using temporary construction again:

     auto that = find(v.begin(), v.end(), target);
    
  • Finally, in the sort call, why aren’t the arguments declared const&? Granted, makes the line even longer as it stands this is inconsistent with the const-correctness illustrated in the find_if call.

  • One small error: Mark the move constructor as noexcept, otherwise there are situations where it won’t be used. The linked case is different since the class has a copy-constructor, yet I can imagine that there are still situations where it matters, especially since your copycon isn’t deleted, just private and undefined.

  • Why is the destructor virtual? If the example is supposed to be minimal then this might be distracting.

  • In generate_it:

     return std::move(Moveable(foo));
    

    The std::move here is redundant, since you are returning a temporary. What’s more, the explicit constructor call is redundant too, since the constructor isn’t marked as explicit. Just return foo; will do.

  • The find_if call could be replaced by vanilla find, using temporary construction again:

     auto that = find(v.begin(), v.end(), target);
    
  • Finally, in the sort call, why aren’t the arguments declared const&? Granted, makes the line even longer as it stands this is inconsistent with the const-correctness illustrated in the find_if call.

  • One small error: Mark the move constructor as noexcept, otherwise there are situations where it won’t be used. The linked case is different since the class has a copy-constructor, yet I can imagine that there are still situations where it matters, especially since your copycon isn’t deleted, just private and undefined.

  • Why is the destructor virtual? If the example is supposed to be minimal then this might be distracting.

  • In generate_it:

     return std::move(Moveable(foo));
    

    The std::move here is redundant, since you are returning a temporary. What’s more, the explicit constructor call is redundant too, since the constructor isn’t marked as explicit. Just return foo; will do.

  • The find_if call could be replaced by vanilla find, using temporary construction again:

     auto that = find(v.begin(), v.end(), target);
    
  • Finally, in the sort call, why aren’t the arguments declared const&? Granted, makes the line even longer as it stands this is inconsistent with the const-correctness illustrated in the find_if call.

Source Link
Konrad Rudolph
  • 6.7k
  • 23
  • 35
  • One small error: Mark the move constructor as noexcept, otherwise there are situations where it won’t be used. The linked case is different since the class has a copy-constructor, yet I can imagine that there are still situations where it matters, especially since your copycon isn’t deleted, just private and undefined.

  • Why is the destructor virtual? If the example is supposed to be minimal then this might be distracting.

  • In generate_it:

     return std::move(Moveable(foo));
    

    The std::move here is redundant, since you are returning a temporary. What’s more, the explicit constructor call is redundant too, since the constructor isn’t marked as explicit. Just return foo; will do.

  • The find_if call could be replaced by vanilla find, using temporary construction again:

     auto that = find(v.begin(), v.end(), target);
    
  • Finally, in the sort call, why aren’t the arguments declared const&? Granted, makes the line even longer as it stands this is inconsistent with the const-correctness illustrated in the find_if call.

lang-cpp

AltStyle によって変換されたページ (->オリジナル) /