Mordante created this revision.
Mordante added reviewers: aaron.ballman, xbolva00, MaskRay.
Mordante added a project: clang.
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D72212
Files:
clang/lib/Sema/SemaStmt.cpp
clang/test/SemaCXX/warn-range-loop-analysis-trivially-copyable.cpp
clang/test/SemaCXX/warn-range-loop-analysis.cpp
Index: clang/test/SemaCXX/warn-range-loop-analysis.cpp
===================================================================
--- clang/test/SemaCXX/warn-range-loop-analysis.cpp
+++ clang/test/SemaCXX/warn-range-loop-analysis.cpp
@@ -19,6 +19,8 @@
struct Foo {};
struct Bar {
+ // The type is too large to suppress the warning for trivially copyable types.
+ char s[128];
Bar(Foo);
Bar(int);
operator int();
Index: clang/test/SemaCXX/warn-range-loop-analysis-trivially-copyable.cpp
===================================================================
--- /dev/null
+++ clang/test/SemaCXX/warn-range-loop-analysis-trivially-copyable.cpp
@@ -0,0 +1,125 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 -Wloop-analysis -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 -Wrange-loop-analysis -verify %s
+
+void test_POD() {
+ struct Record {
+ volatile int a;
+ int b;
+ };
+
+ Record records[8];
+ for (const auto r : records)
+ (void)r;
+}
+
+void test_POD_64_bytes() {
+ struct Record {
+ char a[64];
+ };
+
+ Record records[8];
+ for (const auto r : records)
+ (void)r;
+}
+
+void test_POD_65_bytes() {
+ struct Record {
+ char a[65];
+ };
+
+ // expected-warning@+3 {{loop variable 'r' of type 'const Record' creates a copy from type 'const Record'}}
+ // expected-note@+2 {{use reference type 'const Record &' to prevent copying}}
+ Record records[8];
+ for (const auto r : records)
+ (void)r;
+}
+
+void test_TriviallyCopyable() {
+ struct Record {
+ Record() {}
+ volatile int a;
+ int b;
+ };
+
+ Record records[8];
+ for (const auto r : records)
+ (void)r;
+}
+
+void test_TriviallyCopyable_64_bytes() {
+ struct Record {
+ Record() {}
+ char a[64];
+ };
+
+ Record records[8];
+ for (const auto r : records)
+ (void)r;
+}
+
+void test_TriviallyCopyable_65_bytes() {
+ struct Record {
+ Record() {}
+ char a[65];
+ };
+
+ // expected-warning@+3 {{loop variable 'r' of type 'const Record' creates a copy from type 'const Record'}}
+ // expected-note@+2 {{use reference type 'const Record &' to prevent copying}}
+ Record records[8];
+ for (const auto r : records)
+ (void)r;
+}
+
+void test_NonTriviallyCopyable() {
+ struct Record {
+ Record() {}
+ ~Record() {}
+ volatile int a;
+ int b;
+ };
+
+ // expected-warning@+3 {{loop variable 'r' of type 'const Record' creates a copy from type 'const Record'}}
+ // expected-note@+2 {{use reference type 'const Record &' to prevent copying}}
+ Record records[8];
+ for (const auto r : records)
+ (void)r;
+}
+
+void test_TrivialABI() {
+ struct [[clang::trivial_abi]] Record {
+ Record() {}
+ ~Record() {}
+ volatile int a;
+ int b;
+ };
+
+ Record records[8];
+ for (const auto r : records)
+ (void)r;
+}
+
+void test_TrivialABI_64_bytes() {
+ struct [[clang::trivial_abi]] Record {
+ Record() {}
+ ~Record() {}
+ char a[64];
+ };
+
+ Record records[8];
+ for (const auto r : records)
+ (void)r;
+}
+
+void test_TrivialABI_65_bytes() {
+ struct [[clang::trivial_abi]] Record {
+ Record() {}
+ ~Record() {}
+ char a[65];
+ };
+
+ // expected-warning@+3 {{loop variable 'r' of type 'const Record' creates a copy from type 'const Record'}}
+ // expected-note@+2 {{use reference type 'const Record &' to prevent copying}}
+ Record records[8];
+ for (const auto r : records)
+ (void)r;
+}
Index: clang/lib/Sema/SemaStmt.cpp
===================================================================
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -2779,6 +2779,15 @@
}
}
+/// Determines whether the @p VariableType's declaration is a record with the
+/// clang::trivial_abi attribute.
+static bool hasTrivialABIAttr(QualType VariableType) {
+ if (CXXRecordDecl *RD = VariableType->getAsCXXRecordDecl())
+ return RD->hasAttr<TrivialABIAttr>();
+
+ return false;
+}
+
// Warns when the loop variable can be changed to a reference type to
// prevent a copy. For instance, if given "for (const Foo x : Range)" suggest
// "for (const Foo &x : Range)" if this form does not make a copy.
@@ -2800,10 +2809,12 @@
return;
}
- // TODO: Determine a maximum size that a POD type can be before a diagnostic
- // should be emitted. Also, only ignore POD types with trivial copy
- // constructors.
- if (VariableType.isPODType(SemaRef.Context))
+ // Small trivially copyable types are cheap to copy. Do not emit the
+ // diagnostic for these instances.
+ ASTContext &Ctx = SemaRef.Context;
+ if (Ctx.getTypeSize(VariableType) <= 64 * 8 &&
+ (VariableType.isTriviallyCopyableType(Ctx) ||
+ hasTrivialABIAttr(VariableType)))
return;
// Suggest changing from a const variable to a const reference variable
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
