[Openmp-commits] [openmp] [clang-tidy] Add performance-move-smart-pointer-contents check. (PR #66139)

[via Openmp-commits]

pizzud wrote:

> I don't see problem described by this check as an performance issue. For example:
> 
> ```
> std::unique_ptr<std::vector<int>> ptr;
> std::vector<int> local = std::move(*ptr);
> ```
> 
> No performance issue here, simply value may need to be moved, do not expect that someone have to always keep object in unique_ptr, at some point it may need to be moved/copied. And some types may be cheap to move.
> 
> As for std::shared_ptr, yes this could be a problem.
> 
> My suggestion is:
> 
> * Part related to moving object from a std::shared_ptr should be a bugprone-moved-from-shared check (or some other name).
> * As for current check, I fear that it may cause lot of false-positives, and basicly it should work only on "heavy" types,  but it could be hard to detect such heavy types. But still I don't think why that part should be reduced to just shared ptr, check that would detect std::move calls that are still heavy, simply because moved class is above XYZ bytes in size.
> 

@martinboehme internally suggested putting this in the performance category; perhaps he has a more detailed rationale or some relevant thoughts?

> Fix provided by this check `*std::move(p)` is also questionable... Move will still be called, and this check will still detect such issue, in that case fix does not fix anything.

This check won't re-flag it (see the `correctUnique` and `correctShared` test cases)...but by applying this fix, bugprone-use-after-move will recognize the more standard idiom and can catch the issues it looks at. A pattern like 

```
void f() {
  std::unique_ptr<LargeProtocolBuffer> p = ...;
  LargeProtocolBuffer m = std::move(*p);
  std::cout << p->some_method() << std::endl;
}
```

slipped through bugprone-use-after-move because it assumes the `*std::move()` pattern. Discussion internally led to this check being proposed.

https://github.com/llvm/llvm-project/pull/66139