Page MenuHomePhabricator

Add clang-tidy check for unique_ptr's reset+release→move

Authored by sokolov on Dec 2 2014, 11:51 AM.



Replace x.reset(y.release()); with x = std::move(y);
If y is rvalue, replace with x = y; instead

Diff Detail

Event Timeline

sokolov updated this revision to Diff 16825.Dec 2 2014, 11:51 AM
sokolov retitled this revision from to Add clang-tidy check for unique_ptr's reset+release→move.
sokolov updated this object.
sokolov edited the test plan for this revision. (Show Details)
sokolov added a reviewer: alexfh.
sokolov changed the edit policy from "All Users" to "Administrators".
sokolov added a subscriber: Unknown Object (MLST).
alexfh edited edge metadata.Dec 3 2014, 7:46 AM

Thank you for the check!

Looks generally fine, but needs some polishing to match LLVM style.


Please remove commented lines.


You're already using ast_matchers, so you can avoid it here.


Please use CamelCase for local variables.


In LLVM style single-statement if/for/... bodies don't need braces (and should not be on the same line with the if/for/...).


Clang warnings don't start with a capital letter. We try to move to this style in clang-tidy as well.


Maybe "prefer ptr1 = std::move(ptr2); ..."? It would be more wordy, but also clearer, I think.


LLVM style: one space before comments, please.


Unfortunately, delegated constructors are not implemented in some of the compilers supported by Clang (VS2012). You need to manually delegate the constructor.


Here and below: in LLVM style names of local variables and function parameters should start with a capital letter.

5 calls FileCheck with -implicit-check-not='warning:|error:', so this is superfluous.


You can omit the " [misc-uniqueptr-reset-release]" part in all the messages except for the first one. It will make the test slightly more readable.

sokolov updated this revision to Diff 16915.Dec 4 2014, 1:26 AM
sokolov edited edge metadata.


alexfh accepted this revision.Dec 4 2014, 2:54 AM
alexfh edited edge metadata.

Awesome, thanks!

This revision is now accepted and ready to land.Dec 4 2014, 2:54 AM
alexfh added a comment.Dec 4 2014, 2:55 AM

Do you have commit rights, BTW? If not, I can commit this patch for you.

I don't have commit rights, I didn't submit that many patches so far :)
Please commit.

alexfh closed this revision.Dec 5 2014, 4:00 AM

Committed as r223460 with a few minor changes:
Renamed the test file to be closer to the check name, re-formatted the test to LLVM style and made the CHECK-MESSAGES lines shorter.

alexfh added inline comments.Feb 23 2015, 9:04 AM

The message for this case shouldn't mention std::move. Can you fix this?