[clang] 9c74fb4 - [Sema] Improve -Wrange-loop-analysis warnings.

[Mark de Wever via cfe-commits]

Author: Mark de Wever
Date: 2020-01-11T15:34:02+01:00
New Revision: 9c74fb402e1b7aad4a509a49ab4792154b8ba2c8

URL: https://github.com/llvm/llvm-project/commit/9c74fb402e1b7aad4a509a49ab4792154b8ba2c8
DIFF: https://github.com/llvm/llvm-project/commit/9c74fb402e1b7aad4a509a49ab4792154b8ba2c8.diff

LOG: [Sema] Improve -Wrange-loop-analysis warnings.

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.

Differential Revision: https://reviews.llvm.org/D72212

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

Modified: 
    clang/lib/Sema/SemaStmt.cpp
    clang/test/SemaCXX/warn-range-loop-analysis.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp
index 1e7cc503d8c2..d6c3af9e84c8 100644
--- a/clang/lib/Sema/SemaStmt.cpp
+++ b/clang/lib/Sema/SemaStmt.cpp
@@ -2779,6 +2779,15 @@ static void DiagnoseForRangeReferenceVariableCopies(Sema &SemaRef,
   }
 }
 
+/// 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,13 @@ static void DiagnoseForRangeConstVariableCopies(Sema &SemaRef,
     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. 64 bytes is a common size of a cache line.
+  // (The function `getTypeSize` returns the size in bits.)
+  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

diff  --git a/clang/test/SemaCXX/warn-range-loop-analysis-trivially-copyable.cpp b/clang/test/SemaCXX/warn-range-loop-analysis-trivially-copyable.cpp
new file mode 100644
index 000000000000..f4c76f26f5c9
--- /dev/null
+++ b/clang/test/SemaCXX/warn-range-loop-analysis-trivially-copyable.cpp
@@ -0,0 +1,89 @@
+// 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_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 at +3 {{loop variable 'r' of type 'const Record' creates a copy from type 'const Record'}}
+  // expected-note at +2 {{use reference type 'const Record &' to prevent copying}}
+  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 at +3 {{loop variable 'r' of type 'const Record' creates a copy from type 'const Record'}}
+  // expected-note at +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 at +3 {{loop variable 'r' of type 'const Record' creates a copy from type 'const Record'}}
+  // expected-note at +2 {{use reference type 'const Record &' to prevent copying}}
+  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 at +3 {{loop variable 'r' of type 'const Record' creates a copy from type 'const Record'}}
+  // expected-note at +2 {{use reference type 'const Record &' to prevent copying}}
+  Record records[8];
+  for (const auto r : records)
+    (void)r;
+}

diff  --git a/clang/test/SemaCXX/warn-range-loop-analysis.cpp b/clang/test/SemaCXX/warn-range-loop-analysis.cpp
index c1bcdf6b0ed9..53b0ca288194 100644
--- a/clang/test/SemaCXX/warn-range-loop-analysis.cpp
+++ b/clang/test/SemaCXX/warn-range-loop-analysis.cpp
@@ -20,6 +20,10 @@ struct Container {
 
 struct Foo {};
 struct Bar {
+  // Small trivially copyable types do not show a warning when copied in a
+  // range-based for loop. This size ensures the object is not considered
+  // small.
+  char s[128];
   Bar(Foo);
   Bar(int);
   operator int();