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

[via flang-commits]

martinboehme 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?

While it's true, as @PiotrZSL points out here and in another comment, that a class being "heavyweight" (i.e. expensive to move) is not the only reason to put it behind a `unique_ptr`, I would argue that it's the overwhelmingly common reason.

Quoting a counterexample from a different comment:

```cxx
std::unique_ptr<std::vector<int>> ptr;
std::vector<int> local = std::move(*ptr);
```

Why would someone ever put a `std::vector` behind a `std::unique_ptr` to begin with?[^1] It costs an extra allocation and every access incurs an extra indirection. Granted, moving a `std::unique_ptr` requires copying only a single machine word as opposed to three for a `std::vector`, but surely the cases where this makes `std::unique_ptr<std::vector>` the right tradeoff are the exception?

So I think that if we want to recommend to the user to do `std::move(p)` instead of `std::move(*p)`, it's because we assume that it's cheaper to move the `unique_ptr` rather than the contents. There's a question of how often this recommendation will be right (I think it will very rarely be wrong), but in any case, the reason we're making the recommendation is performance, so this seems the right category for the check.

`shared_ptr`, on the other hand, is definitely a different case, and I agree that "bugprone" seems like a better category.

[^1]: Let's disregard custom deleters. These show up as a template argument, so if we're concerned about them, we should simply check that there is no custom deleter.

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