[PATCH] D34114: [clang] Change the condition of unnecessary packed warning

[Chih-Hung Hsieh via Phabricator via cfe-commits]

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