9

I have a function that gets a "Config" struct, which at the moment is simply a struct that contains an array

class ShmConfig {
public:
   std::int64_t shellShmSize[ShellId_Count];
};

And I pass it on to another function in the function with std::move, because I want all code places that copy it be agnostic to the fact that it can efficiently be copied.

m_allocator.setup(std::move(config));

If later it can be more efficient to move it (maybe because I add an std::string to it), it will automatically be become more efficient. But clang-tidy advises against it

std::move of the variable 'config' of the trivially-copyable type 'ShmConfig' has no effect; remove std::move()

I don't understand why this is desirable. Is there any disadvantage to wrapping it with move? Are these advices to be followed only if one runs clang-tidy regularly, so that one can spot cases of where std::move will make sense later on, upon data member additions?

Johannes Schaub - litb
  • 496,577
  • 130
  • 894
  • 1,212
  • 1
    in general code is not agnostic to whether an object has been moved from or not. Depends on whether you use `config` after `std::move(config)` – 463035818_is_not_an_ai May 18 '21 at 12:01
  • 3
    Since `ShmConfig` has no managed resources, `move` semantics will have no relevance. Adding `std::move` *just in case* some future version of `ShmConfig` has a move-able member variable resource seems like wa-a-a-a-ay premature optimization. YAGNI. – Eljay May 18 '21 at 12:02
  • 11
    I disagree with clang-tidy. The `std::move` has a very discernable effect. It tells future me that I don't care about the value of the variable anymore, and shouldn't use it other than for re-assignment. It's that *explicit* annotation that even allows the optimization that comes with moving. It's a shame it was called `move` and not `may_move_now`. I guess the latter just wasn't as catchy. – StoryTeller - Unslander Monica May 18 '21 at 12:09
  • I feel I should also point out, that `clang-tidy` can sometimes give not-the-best advice https://stackoverflow.com/q/52871026/817643 – StoryTeller - Unslander Monica May 18 '21 at 12:12
  • Some classes (like `unique_ptr`) come with defined moved-from states. It is not uncommon to check for the moved-from state as a signal that the object has "already been used". Someone accustomed to this pattern might do `/* set up if not already set up */ if (is_ready(config)) m_allocator.setup(std::move(config))` to detect whether the config has been consumed by a previous call to `setup`. (More common if `ShmConfig` has an `operator bool()` conversion.) The warning is saying, "Hey, this object may not behave the way you expect." – Raymond Chen May 18 '21 at 13:05
  • @Eljay `Adding std::move just in case some future version`: But it's not only a premature optimization, it's semantics. As the developer, you want to express something with that conceptionally, fully correct on a more abstract layer. I fully agree with StoryTeller here, that the origin of the confusion lies within the misleading chosen terminology for `std::move`. At least for classes/structs that might likely be changed/extended in future, one should prefer correct semantics over code restriction to effective compiler behavior I think. – Secundi May 27 '21 at 08:52
  • WG21 probably chose `std::move` because `std::permission_to_transplant_innards_leaving_the_original_object_in_a_valid_but_unspecified_state` may be a little too verbose. – Eljay May 27 '21 at 12:20
  • I did not say, that finding the right naming would be an easy job here... :) But since `std::move` doesn't really move anything in general besides the issues mentioned here, it's quite reasonable to rethink about the terminology. – Secundi May 28 '21 at 06:40
  • @Sec does `std::forward` really forward, or does it just do a cast? – Johannes Schaub - litb May 28 '21 at 07:39
  • Is cppreference's description of it even correct? "this overload forwards the argument to another function with the value category it had when passed to the calling function.". Rage! – Johannes Schaub - litb May 28 '21 at 07:41
  • It's (almost?) always a question of balance where to start with simplifications in terms of terminology. Since the namings `std::move` and `std::forward` are still quite good compromises here with only a few exceptional cases where you have to rethink about their actual behavior, clang-tidy overreacts here by far since it solely refers to effective behavior without a required focus on semantics. It's somehow not really consequent since it doesn't(?) complain about the actual cast but the method call itself, that is solely a semantical wrapper actually. – Secundi May 28 '21 at 11:24
  • With a view on responsibilities, things might become much clearer here I think. Imagine a complex logic layer that handles a mass of (possibly generated) structs/interfaces. As the according developer of the logic, you might give a promise about move-semantics for several parts of it. Since the operation is well-defined even for PODs, solely might not affect the performance compared to copies for all cases in doubt, there is no benefit to provide a possibly complex branching here for those cases, but only disadvantages (boilerplate...) in doubt, for types that are not really your beer in doubt – Secundi May 28 '21 at 11:31
  • Clang-tidy should focus on really relevant cases, return value handling for instance in combination with `std::move` where the impact on performance can be relevant, theoretically and practically. – Secundi May 28 '21 at 11:32
  • It overreacts on a whole lot of other cases. For example if I do "std::move" and the destination parameter type is a "const T&" it recommends to drop the "std::move", because behavior is as if I omitted it. IMO bad recommendations! Therefore I suspect it's supposed to be run often enough to make it suggest adding it again when needed. – Johannes Schaub - litb May 28 '21 at 11:36
  • 1
    Not really an answer to the question, but in a similar scenario I found that the warning goes away if I add explicit move constructor to the class, for example: `ShmConfig(ShmConfig&& o) noexcept = default;`. – MartinBG Nov 10 '21 at 20:07

1 Answers1

1

Yes, I believe your gratuitous and ubiquitous std::move()'s are a code smell; but - perhaps not mainly for the reason you asked about.

Using std::move(), for classes which don't have externally-visible side-effects to copying or moving, is about improving performance by avoiding copies. But - this is your app's configuration we're talking about, not its large input data or computed structures. It is unlikely that reading your configuration has a significant, impact on your app's performance; it may not even be noticeable; and it's not like you're reading the configuration a million times. So why are you even bothering with the minor details here? Have you profiled your application? Can peppering your code with std::move() help you, even in principle? Unlikely.

Also,

If later it can be more efficient to move it (maybe because I add an std::string to it),

Let me remind you of two pearls of wisdom:

  • Premature optimization is the root of all evil --Donald Knuth
  • Software that is worth writing, is worth rewriting --Folklore

So, if something changes later, rewrite your code later. It's not like your efforts right now are expended in making your code more elegant, readable etc. And - maybe your whole configuration handling will change and this will be irrelevant.

einpoklum
  • 118,144
  • 57
  • 340
  • 684