1588744 - Use of std::move for trivially copyable types

Status: NEW

(Core :: Storage: IndexedDB, task, P3)

Description

[Simon Giesecke [:sg] [he/him]]

This is based on a review discussion around https://phabricator.services.mozilla.com/D46945#inline-299268.

There is a clang-tidy analyis that identifies uses of std::move that have no effect because the type is trivially copyable. While this is correct, it is not clear if there should be a std::move anyway. Points to consider here:

• The type may change to be no longer trivially copyable, in which case the std::move starts to have an effect, which might go unnoticed. Even if that is identified by another clang-tidy check, it will not be noticed in the same way, as the review bot only reports issues with the code locations that are changed, not with code locations that use that code.
• It might depend on the type how to handle is. The property of being trivially copyable might or might not be an implementation detail. In this case (JS::Handle<JSObject*>), it is unclear if that's the case. It is not explicitly documented that it is trivially copyable, and from reading the definition, as it derives from some other template, it is not immediately clear. On the other hand, it might be assumed to be implicitly clear that a "handle" is trivially copyable.
• It might be considered good style to indicate that the value is logically moved, even if it will never have an effect on code generation.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

It turns out there was a bug filed on this already and I NI'ed Ehsan and his decision at https://bugzilla.mozilla.org/show_bug.cgi?id=1558359#c4 is to turn off CheckTriviallyCopyableMove, so I think we can consider this addressed when that bug is fixed. Thanks very much for spinning this off into a well thought-out bug! (Of which I'm very much in agreement!)

Comment 2

[Simon Giesecke [:sg] [he/him]]

Yes, turning off this specific analysis on Phabricator appears to make sense to me. Still, this leaves the question open how we want to write such code.

A related question is whether to place a std::move for passing arguments when the function accepts a const&. std::move also has no effect there, which is also reported by a clang-tidy analysis that is active on Phabricator. Again, the function signature may also change to a value, and in that case adding a std::move at the call sites might be beneficial (it might even be required if the type is not copyable, but that is beyond stylistic matters). So again there is the question if we should encourage adding a std::move proactively.

I tend to vouch for a rule like this for both cases:

When you are passing an argument to a function, which

• is not obviously a trivially copyable type which is highly unlikely to change to a non-trivially copyable type, and
• is a modifiable lvalue, and
• is not used by the caller after the function call,
• (is not passed as a non-const reference), [this is actually redundant as non-const reference parameters are not allowed by the coding style]
  pass it with std::move, even if it has no effect at the moment of writing.

So, e.g.

template<typename T, typename U>
std::enable_if_t<std::is_integral_v<U>> foo() {
  int i = 4;
  f1(i); // no std::move, because i is trivially copyable and the type is specified locally

  T x;
  f2(std::move(x)); // std::move because we know nothing about T

  U y = i;
  f3(y); // no std::move because we know that U is integral and therefore trivially copyable

  RefPtr<Foo> z;
  f4(std::move(z)); // std::move because z is not trivially copyable

  const RefPtr<Foo> z2;
  f4(z2); // no std::move because z2 is const
  // BUT it should be considered to change z2 to be non-const in this case (out of scope of this rule though)

  FooType foo;
  f5(std::move(foo)); // std::move because without further information, it is not obvious that FooType is trivially copyable
  
  typename U::size_type size;
  f6(size); // no std::move because a "size_type" is expected to be trivially copyable
}