[Sema] Improve -Wrange-loop-analysis warnings

[Mordante]

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.

[merge_guards_bot]

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]

clang/lib/Sema/SemaStmt.cpp

2806

assert it?

[Mordante]

clang/lib/Sema/SemaStmt.cpp

2806

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

[xbolva00]

clang/lib/Sema/SemaStmt.cpp

2806

if (isTriviallyCopyableType) {

assert(isPOD);

}

But probably not very useful to have an assert here.

[Mordante]

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.

[MaskRay]

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]

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

[MaskRay]

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]

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.

[MaskRay]

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]

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]

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.

clang/lib/Sema/SemaStmt.cpp

if (ReturnsReference) {
     QualType NonReferenceType = VariableType.getNonReferenceType();
     NonReferenceType.removeLocalConst();
     SemaRef.Diag(VD->getBeginLoc(), diag::note_use_non_reference_type)
         << NonReferenceType << VD->getSourceRange()
         << FixItHint::CreateRemoval(VD->getTypeSpecEndLoc());
   }
 }
 
+/// 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.
 static void DiagnoseForRangeConstVariableCopies(Sema &SemaRef,
                                                 const VarDecl *VD) {
   const Expr *InitExpr = VD->getInit();
   if (!InitExpr)
     return;
 
   QualType VariableType = VD->getType();
 
   if (const CXXConstructExpr *CE = dyn_cast<CXXConstructExpr>(InitExpr)) {
     if (!CE->getConstructor()->isCopyConstructor())
       return;
   } else if (const CastExpr *CE = dyn_cast<CastExpr>(InitExpr)) {
     if (CE->getCastKind() != CK_LValueToRValue)
       return;
   } else {
     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
   // if doing so will prevent a copy.
   SemaRef.Diag(VD->getLocation(), diag::warn_for_range_copy)
       << VD << VariableType << InitExpr->getType();
   SemaRef.Diag(VD->getBeginLoc(), diag::note_use_reference_type)
       << SemaRef.Context.getLValueReferenceType(VariableType)
       << VD->getSourceRange()

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

+// 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@+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_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_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;
+}

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

struct Container {
   typedef Iterator<T> I;
 
   I begin();
   I end();
 };
 
 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();
 };
 
 // Testing notes:
 // test0 checks that the full text of the warnings and notes is correct.  The
 //   rest of the tests checks a smaller portion of the text.