[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added a comment.

>> It is also completely irrelevant, because a new programmer will not understand that const T const * is actually T const* and not T const * const. An experienced programmer can understand it well either way.
>
> not sure I follow the argument here, but surely I also agree that east consts are more readable and fool-proof. it's unfortunate that most of the C++ world is stuck with the west consts.

Exactly this.

> surely, it would be nice to have a clang-tidy check or something (by design clang-format only performs white-space changes).

It would also be good to have some sort of refactoring choice in clangd to swap the alignment. 
Maybe we should add a feature request on GitHub for both.

> FWIW, one reason this isn't enabled is that it's relatively recent and also not completely safe in principle.

It's not as simple as "if LLVM cares about const-alignment, this flag should be on".

I also was curious about that, until I've read the documentation about this formatter option. But is it safe for already correct alignments?
Then it could be used in the linting step to just verify, that it is done correctly, am I right?

Back to topic
-------------

I have some open questions:

@njames93 asked me to remove the explicitly deleted copy/move constructors/assignment operators.
But this would break the rule of 5, or I have to move the `CFG.h` include into the header which is in my opinion also against the coding convention.
Is it ok to keep them, should I break the rule of 5 or should I move the `CFG.h` include into the header?

@njames93 also mentioned, that the complete traversal of the Context is a waste of resources. 
But I have no clue, how to use the Context down from the top function.
Is there even a way to do so, or do I have to start with `TraverseFunctionDecl` in the `RecursiveASTVisitor`?

An even better approach would be to start with the DeclRefExpr itself. 
Is there a way to traverse a matcher in the reverse order from the DeclRefExpr as start?
Or do I have to parse it by hand then?

Currently, I got many reviews regarding the coding stile and the result, but I would also like to see a review to the CFG parsing itself.

There are also "open improvements", but they require a huge amount of work on top of this patch:

- fixups in lambda captures:
  - To fixup lambda captures sustainably, like the handling of multiple last usages in the capture list / implicit capture. The new capture list must be built at the end of the parsing of the translation unit.
  - Some sort of capture-list builder must be used, which first collects all last usages, and then creates one single fix up for all.
  - In my opinion, this is non-blocking since the benefit is small compared to the work.
  - I am willing to add this, if desired, because I have a plan how I would implement it.
- A heuristic to catch copyable but non trivially copyable and movable "guards".
  - This is a lot harder, and I actually have no clue how to do this without scanning each type __recursively__ for side effects to child fields in any destructor which are not directly followed by memory management (destruction).
  - I would like to defer this.
- There is no detection of last usages of fields
  - This is also non-blocking, since it is a pure feature.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137205/new/

https://reviews.llvm.org/D137205