Page MenuHomePhabricator

[Sema] Improve -Wrange-loop-analysis warnings
ClosedPublic

Authored by Mordante on Jan 4 2020, 12:49 PM.

Details

Summary

No longer generate a diagnostic when a small trivially copyable type is used without a reference. Before the test looked for a POD type and had no size restriction. Since the range-based for loop is only available in C++11 and POD types are trivially copyable in C++11 it's not required to test for a POD type.

Since copying a large object will be expensive its size has been restricted. 64 bytes is a common size of a cache line and if the object is aligned the copy will be cheap. No performance impact testing has been done.

Diff Detail

Event Timeline

Mordante created this revision.Jan 4 2020, 12:49 PM

Unit tests: pass. 61248 tests passed, 0 failed and 736 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

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

xbolva00 added inline comments.Jan 4 2020, 1:25 PM
clang/lib/Sema/SemaStmt.cpp
2806

assert it?

Mordante marked an inline comment as done.Jan 4 2020, 1:43 PM
Mordante added inline comments.
clang/lib/Sema/SemaStmt.cpp
2806

Sorry I don't understand what you mean. What do you want to assert?

xbolva00 added inline comments.Jan 4 2020, 3:42 PM
clang/lib/Sema/SemaStmt.cpp
2806

if (isTriviallyCopyableType) {

assert(isPOD);

}

But probably not very useful to have an assert here.

Mordante marked 3 inline comments as done.Jan 5 2020, 5:10 AM
Mordante added inline comments.
clang/lib/Sema/SemaStmt.cpp
2806

That will not work. Every POD type is TriviallyCopyable, but not every TriviallyCopyable type is a POD type.

In the tests I have separate tests for POD types and TriviallyCopyable types.

Thanks for the patch. Will take some time to review. Without this, adding -Wrange-loop-analysis to -Wall will make lots of projects fail to build if they have -Werror. For example, for (const absl::string_review : ...) or const std::string_view is common.

Mordante marked an inline comment as done.Jan 7 2020, 11:26 AM

You're welcome. It would be nice if we can get this one in the release.

MaskRay added inline comments.Jan 7 2020, 1:44 PM
clang/test/SemaCXX/warn-range-loop-analysis-trivially-copyable.cpp
7

test_POD can be dropped. It does not add test coverage.

"Struct with a volatile member no longer a POD" is a MSVC bug. https://stackoverflow.com/questions/59103157/struct-with-a-volatile-member-no-longer-a-pod-according-to-msvc

38

test_TriviallyCopyable can also be dropped.

Or you can add volatile to test_TriviallyCopyable_64_bytes

89

test_TrivialABI can be dropped. It does not add test coverage.

MaskRay accepted this revision.Jan 7 2020, 1:47 PM

Commit eec0240f97180ea876193dcfa3cb03cb652d9fe3 "Adds -Wrange-loop-analysis to -Wall" was premature before this fix, as it could cause lots of false positives. I think a few tests are redundant. Other than those, LG.

This revision is now accepted and ready to land.Jan 7 2020, 1:47 PM
MaskRay added inline comments.Jan 7 2020, 1:47 PM
clang/test/SemaCXX/warn-range-loop-analysis.cpp
23

too large to suppress means it does not suppress the warning in English, I think.

rtrieu added inline comments.Jan 8 2020, 7:07 PM
clang/lib/Sema/SemaStmt.cpp
2812–2819

You should add why 64 bytes was chosen to this comment to both explain the 64 * 8 magic number in the following lines, and to let people reading this code know why that was picked without needed to look up the patch notes.

Mordante marked 8 inline comments as done.Jan 11 2020, 6:31 AM
Mordante added inline comments.
clang/lib/Sema/SemaStmt.cpp
2812–2819

I was not sure whether we also wanted this information in the comment. I'll add more information to the comment.

clang/test/SemaCXX/warn-range-loop-analysis-trivially-copyable.cpp
7

I'll remove these tests. I think there are other tests that test what is and is not a POD.

clang/test/SemaCXX/warn-range-loop-analysis.cpp
23

I'll clarify the text.

This revision was automatically updated to reflect the committed changes.
Mordante marked 3 inline comments as done.