chh added inline comments.
================ Comment at: lib/AST/RecordLayoutBuilder.cpp:1887 + // greater than the one after packing. + if (Packed && UnpackedAlignment <= Alignment) Diag(D->getLocation(), diag::warn_unnecessary_packed) ---------------- chh wrote: > With this change, UnpackedSize is unused and caused a warning about > unused-but-set-variable. Please use it or remove the variable. > > I think UnpackedSizeInBits should be used somehow in the condition, because > bit fields can be affected by the packed attribute. Please add a few unit > test cases with bit fields. For example, the following cases showed that S21 > is affected by packed, but got a wrong "unnecessary" warning after this > change. > > > ``` > struct S21 { > unsigned char a:6; > unsigned char b:6; > } __attribute__((packed, aligned(1))); > struct S22 { > unsigned char a:6; > unsigned char b:6; > } __attribute__((aligned(1))); > struct S23 { > unsigned char a:6; > unsigned char b:6; > }; > > ``` > Warnings: > > ``` > padding size of 'S21' with 4 bits to alignment boundary > packed attribute is unnecessary for 'S21' > padding struct 'S22' with 2 bits to align 'b' > padding size of 'S22' with 2 bits to alignment boundary > padding struct 'S23' with 2 bits to align 'b' > padding size of 'S23' with 2 bits to alignment boundary > > ``` > I am still getting warning about unused-but-set-variable UnpackedSize. ================ Comment at: lib/AST/RecordLayoutBuilder.cpp:636 + /// \brief the flag of field offset changing due to packed attribute. + bool IsFieldOffsetChangedWithPacked; + ---------------- s/IsFieldOffsetChangedWithPacked/HasPackedField/ ================ Comment at: lib/AST/RecordLayoutBuilder.cpp:1985 + // If the offset of any field changed due to packed, then the packed is not + // necessary. + if (isPacked && Offset != UnpackedOffset) { ---------------- "then packed is not necessary" is incorrect. Maybe this comment can be removed since the code is clear enough. ================ Comment at: test/CodeGenCXX/warn-padded-packed.cpp:152 +void f(S1*, S2*, S3*, S4*, S5*, S6*, S7*, S8*, S9*, S10*, S11*, S12*, S13*, S14*, S15*, + S16*, S17*, S18*, S19*, S20*, S21*, S22*, S23*, S24*, S25*, S26*, S27*){} ---------------- Please wrap line 150, 151, 152 to less than 80 chars per line. The only exceptions in this file are the 'expected-warning' lines. https://reviews.llvm.org/D34114 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits