This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Fix bugprone-use-after-move when move is in noexcept operator
ClosedPublic

Authored by njames93 on Jan 26 2020, 5:37 PM.

Diff Detail

Event Timeline

njames93 created this revision.Jan 26 2020, 5:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 26 2020, 5:37 PM
njames93 added a project: Restricted Project.
Quuxplusone added inline comments.
clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
408

What about sizeof, alignof, requires, typeid, and other such unevaluated contexts? Shouldn't there be a common way to spell "this expression is unevaluated"? (I don't know if there is or not.)

Unit tests: pass. 62199 tests passed, 0 failed and 815 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

gribozavr2 added inline comments.Jan 26 2020, 11:26 PM
clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
408

+1 to adding handling and tests for other unevaluated contexts.

clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
1276

Is the macro a necessary part of this test? If not, can it be removed?

njames93 marked 2 inline comments as done.Jan 27 2020, 1:13 AM
njames93 added inline comments.
clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
408

I think i can write a matcher that will detect that

clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
1276

I like to stick to the bug report as much as possible

gribozavr2 added inline comments.Jan 27 2020, 2:12 AM
clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
1276

This is not generally the policy in LLVM and Clang. We prefer minimal reproducers.

njames93 updated this revision to Diff 240506.Jan 27 2020, 2:29 AM
  • added more unevaluated context checks
njames93 marked 3 inline comments as done.Jan 27 2020, 2:31 AM
njames93 added inline comments.
clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
1276

I've kept it minimal, the REQUIRE definition is just to suppress other warnings about unused expressions

njames93 updated this revision to Diff 240507.Jan 27 2020, 2:32 AM
  • Elide braces

Unit tests: pass. 62207 tests passed, 0 failed and 815 were skipped.

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Unit tests: pass. 62206 tests passed, 0 failed and 815 were skipped.

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

njames93 updated this revision to Diff 240512.Jan 27 2020, 3:00 AM
  • Fix formatting

Unit tests: pass. 62207 tests passed, 0 failed and 815 were skipped.

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

gribozavr2 accepted this revision.Jan 27 2020, 4:11 AM

LGTM with fixes to the test.

clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
1276

Adding (void) should do the trick.

1279

I'm not sure why foo and otherUnenvaluated are separate functions. If you want to separate them, I think it would make more sense to put each operator into a separate function then.

This revision is now accepted and ready to land.Jan 27 2020, 4:11 AM
njames93 updated this revision to Diff 240547.Jan 27 2020, 6:05 AM
  • Fix nits in test cases
njames93 marked 4 inline comments as done.Jan 27 2020, 6:06 AM

Unit tests: pass. 62213 tests passed, 0 failed and 815 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

njames93 updated this revision to Diff 240718.Jan 27 2020, 4:22 PM
  • Remove artifacts of old dependent review

Unit tests: pass. 62249 tests passed, 0 failed and 816 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

This revision was automatically updated to reflect the committed changes.