[PATCH] D72212: [Sema] Improve -Wrange-loop-analysis warnings

[Mark de Wever via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Mordante marked 3 inline comments as done.
Closed by commit rG9c74fb402e1b: [Sema] Improve -Wrange-loop-analysis warnings. 
(authored by Mordante).

Changed prior to commit:
  https://reviews.llvm.org/D72212?vs=236199=237497#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72212/new/

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
@@ -20,6 +20,10 @@
 
 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();
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,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@+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;
+}
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();
+
+  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  : Range)" if this form does not make a copy.
@@ -2800,10 +2809,13 @@
 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  = 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

[PATCH] D72212: [Sema] Improve -Wrange-loop-analysis warnings

[Mark de Wever via Phabricator via cfe-commits]

Mordante marked 8 inline comments as done.
Mordante added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:2812-2813
 
-  // 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  = SemaRef.Context;

rtrieu wrote:
> 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.
I was not sure whether we also wanted this information in the comment. I'll add 
more information to the comment.



Comment at: clang/test/SemaCXX/warn-range-loop-analysis-trivially-copyable.cpp:6
+  struct Record {
+volatile int a;
+int b;

MaskRay wrote:
> `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
I'll remove these tests. I think there are other tests that test what is and is 
not a POD.



Comment at: clang/test/SemaCXX/warn-range-loop-analysis.cpp:22
 struct Bar {
+  // The type is too large to suppress the warning for trivially copyable 
types.
+  char s[128];

MaskRay wrote:
> `too large to suppress` means it does not suppress the warning in English, I 
> think.
I'll clarify the text.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72212/new/

https://reviews.llvm.org/D72212



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72212: [Sema] Improve -Wrange-loop-analysis warnings

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:2812-2813
 
-  // 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  = SemaRef.Context;

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72212/new/

https://reviews.llvm.org/D72212



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72212: [Sema] Improve -Wrange-loop-analysis warnings

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/test/SemaCXX/warn-range-loop-analysis.cpp:22
 struct Bar {
+  // The type is too large to suppress the warning for trivially copyable 
types.
+  char s[128];

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72212/new/

https://reviews.llvm.org/D72212



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72212: [Sema] Improve -Wrange-loop-analysis warnings

[Fangrui Song via Phabricator via cfe-commits]

MaskRay accepted this revision.
MaskRay added a comment.
This revision is now accepted and ready to land.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72212/new/

https://reviews.llvm.org/D72212



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72212: [Sema] Improve -Wrange-loop-analysis warnings

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/test/SemaCXX/warn-range-loop-analysis-trivially-copyable.cpp:6
+  struct Record {
+volatile int a;
+int b;

`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



Comment at: 
clang/test/SemaCXX/warn-range-loop-analysis-trivially-copyable.cpp:37
+
+void test_TriviallyCopyable() {
+  struct Record {

`test_TriviallyCopyable` can also be dropped.

Or you can add volatile to `test_TriviallyCopyable_64_bytes`



Comment at: 
clang/test/SemaCXX/warn-range-loop-analysis-trivially-copyable.cpp:88
+
+void test_TrivialABI() {
+  struct [[clang::trivial_abi]] Record {

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72212/new/

https://reviews.llvm.org/D72212



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72212: [Sema] Improve -Wrange-loop-analysis warnings

[Mark de Wever via Phabricator via cfe-commits]

Mordante marked an inline comment as done.
Mordante added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72212/new/

https://reviews.llvm.org/D72212



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72212: [Sema] Improve -Wrange-loop-analysis warnings

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72212/new/

https://reviews.llvm.org/D72212



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72212: [Sema] Improve -Wrange-loop-analysis warnings

[Mark de Wever via Phabricator via cfe-commits]

Mordante marked 3 inline comments as done.
Mordante added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:2806
-  // constructors.
-  if (VariableType.isPODType(SemaRef.Context))
 return;

xbolva00 wrote:
> Mordante wrote:
> > xbolva00 wrote:
> > > assert it?
> > Sorry I don't understand what you mean. What do you want to assert?
> if (isTriviallyCopyableType) {
>  assert(isPOD);
> }
> 
> But probably not very useful to have an assert here.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72212/new/

https://reviews.llvm.org/D72212



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72212: [Sema] Improve -Wrange-loop-analysis warnings

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:2806
-  // constructors.
-  if (VariableType.isPODType(SemaRef.Context))
 return;

Mordante wrote:
> xbolva00 wrote:
> > assert it?
> Sorry I don't understand what you mean. What do you want to assert?
if (isTriviallyCopyableType) {
 assert(isPOD);
}

But probably not very useful to have an assert here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72212/new/

https://reviews.llvm.org/D72212



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72212: [Sema] Improve -Wrange-loop-analysis warnings

[Mark de Wever via Phabricator via cfe-commits]

Mordante marked an inline comment as done.
Mordante added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:2806
-  // constructors.
-  if (VariableType.isPODType(SemaRef.Context))
 return;

xbolva00 wrote:
> assert it?
Sorry I don't understand what you mean. What do you want to assert?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72212/new/

https://reviews.llvm.org/D72212



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72212: [Sema] Improve -Wrange-loop-analysis warnings

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:2806
-  // constructors.
-  if (VariableType.isPODType(SemaRef.Context))
 return;

assert it?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72212/new/

https://reviews.llvm.org/D72212



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72212: [Sema] Improve -Wrange-loop-analysis warnings

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 61248 tests passed, 0 failed 
and 736 were skipped.

{icon times-circle color=red} clang-tidy: fail. Please fix clang-tidy findings 
.

{icon times-circle color=red} 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 



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72212/new/

https://reviews.llvm.org/D72212



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72212: [Sema] Improve -Wrange-loop-analysis warnings

[Mark de Wever via Phabricator via cfe-commits]

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();
+
+  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  : Range)" if this form does not make a copy.