Fixed remarks from Aaron.

What do you think of it?

Handle ref & val mixed cases.

Extend the context.

In D91037#2391915, @aaron.ballman wrote:

LGTM!

Fixed fix-it section.

Use IgnoreParenImpCasts

In D91037#2387377, @njames93 wrote:

Taking a step back, rather than worrying about if its an ExprWithCleanups. Shouldn't we just get the condition removing all implicit nodes.

const Expr* Cond = InnerIf->getCond()->ignoreImplicit();

This has to be simpler and will likely future proof this against any other implicit nodes potentially added in future that can appear between the Condition and the binary operator.

Simplified to use IgnoreImplicit().

dyn_cast_or_null -> dyn_cast

*iterator -> getSubExpr()

auto -> const auto.

In D91037#2381606, @njames93 wrote:

Think this has an incorrect name, seems to have something to do with bugprone-redundant-branch-condition

In D89194#2323917, @JonasToth wrote:

I am fine now, thank you!

In D89194#2323868, @JonasToth wrote:

LGTM.
Short reminder to please use full context patches. Its not an issue right now, but review sometimes just needs the context to understand the changes :)

Full diff.

Done. Any other comments/ideas?

• Updated docs on the matcher.
• Register the new matcher.

Done.

• Fixed the test.
• Fixed clang-format issues.

In D80896#2076735, @njames93 wrote:

How are you landing these changes, because the commit message isn't lining up with the PR name?

Simplify test.

@njames93, @aaron.ballman do you have any other comments?

• Fix test.
• Simplify the code.

Fix and simplify the condition.

Fix formatting.

@njames93, @aaron.ballman do you have any other questions/comments?

In D80536#2055500, @njames93 wrote:

I'm having trouble with the reproduction of this - https://godbolt.org/z/tsMfcj.
Aside from that this needs some test cases to demonstrate the patch is indeed working

One more gentle ping)

Rebase over the current master.

Gentle ping)

In D74692#1923191, @Quuxplusone wrote:

I still think this entire patch is misguided; there's no reason to make the note for const std::string s; std::move(s) any longer than the note for int i; std::move(i) or volatile std::string v; std::move(v). People should not be using moved-from objects, period; and those who want to use moved-from objects, should not enable this clang-tidy check.

However, I have no further comments besides philosophical opposition to the whole idea.

Removed top-level consts.

I'm done, Aaron, Quuxplusone, do you have any comments?

In D74692#1911150, @Quuxplusone wrote:

Anyway, I still don't see the point of this patch. It seems like you're just duplicating the work of performance-move-const-arg. People who want to be shown notes about moves-of-const-args should just enable that check instead.

Cosmetic and style fixes.

Any further comments?

Typo fixed.

1. Special handling for captured variables in lambdas,
2. messages texts were changed.

Warning on use after move is still generated. But there's also an additional note

In D74692#1884054, @Quuxplusone wrote:

(I sent this to the mailing list, but I guess it doesn't show up here unless I do it through Phab. Quoting myself—)

I see your point about how users who care should always be passing this check alongside "performance-move-const-arg"; but IMHO it still makes sense for clang-tidy to warn unconditionally about code of the form

x = std::move(y);
use(y);

regardless of the incidental type of y. Sure, it's technically not wrong if y is const... or if y is a primitive type, such as Widget*... or if y is a well-defined library type, such as std::shared_ptr<Widget>... or if y is a library type that works in practice, such as std::list<int>... but regardless of its technical merits, the code is still logically semantically wrong, and I think that's what the clang-tidy check should be trying to diagnose.

In D74692#1879039, @mboehme wrote:

Even if no move actually happens, the code is still wrong

In D57662#1797607, @derek wrote:

@zinovy.nis @alexfh @JonasToth @MyDeveloperDay

I'd be happy to provide a patch.