This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

Reviewers
alexfh
Summary

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.

clang-tidy/misc/UniqueptrResetRelease.cpp
15

Please remove commented lines.

40

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

41

Please use CamelCase for local variables.

56

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

69

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

69

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

75

LLVM style: one space before comments, please.

clang-tidy/misc/UniqueptrResetRelease.h
29

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

30

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

test/clang-tidy/uniqueptr-reset-release.cpp
5

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

36

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.

Done

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
test/clang-tidy/uniqueptr-reset-release.cpp
36

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