[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 &Ctx = 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