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


================
Comment at: clang/test/Sema/struct-packed-align.c:170
+#elif defined(_AIX)
+// On AIX, [bool, char, short] bitfields have the same alignment as unsigned
+// int.
----------------
aaron.ballman wrote:
> We're not really testing the behavior of `bool` or `short` anywhere and it'd 
> be nice to verify that. Perhaps instead of modifying an existing test to add 
> more fields, it'd make sense to make a new test structure?
> 
> While thinking of other potentially smaller-than-int types, I wondered 
> whether `wchar_t` has special behavior here as well (I have no idea how that 
> type is defined for AIX, so it's entirely possible it's size and alignment 
> already match `int`).
> We're not really testing the behavior of bool or short anywhere and it'd be 
> nice to verify that. 

The comment is to explain why char has 4-byte alignment mainly. And the 
testcase here is, as comments mentioned, to test `Packed attribute shouldn't be 
ignored for bit-field of char types`.  Perhaps I should remove `bool` and 
`short` so that people wouldn't be confused.  

And the special alignment regarding bool, short etc. has been tested when the 
special rule introduced on aix here: https://reviews.llvm.org/D87029.



> Perhaps instead of modifying an existing test to add more fields, it'd make 
> sense to make a new test structure?

I don't think it's necessary to make a new test structure. The modified 
testcase test the same property as the original one. And it is more capable as 
it can also test the property for AIX target.




> I wondered whether wchar_t has special behavior here as well 

I think `wchar_t` has the same special behavior. Basically any type smaller 
than 4 bytes will be aligned to 4 when it comes to bitfield. Please correct me 
if I am wrong @hubert.reinterpretcast 




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

https://reviews.llvm.org/D102715

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

Reply via email to