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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits