Xiangling_L marked 9 inline comments as done.
Xiangling_L added inline comments.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:893
+def warn_pragma_pack_identifer_not_supported : Warning<
+ "specifying an identifier within pragma pack is not supported, identifier is
ignored">,
+ InGroup<PragmaPack>;
----------------
aaron.ballman wrote:
> If the user wrote an identifier, it seems like there's a strong chance that
> ignoring the identifier will result in incorrect behavior that would lead to
> hard-to-find ODR issues. Should this scenario be an error rather than a
> warning where the identifier is ignored?
Could you show a more concrete example or give more details on how possible
incorrect behavior would lead to hard-to-find ODR issues?
================
Comment at: clang/include/clang/Sema/Sema.h:486
+ : PackAttr(true), AlignMode(M), PackNumber(Num), XLStack(IsXL) {
+ assert(Num == PackNumber && "Unexpected value.");
+ }
----------------
aaron.ballman wrote:
> The string literal here doesn't really convey what's being asserted -- it
> took me a while to figure out that this is trying to catch truncation issues
> when `Num` cannot be represented by an `unsigned char`.
Sorry for that, I would make it clearer.
================
Comment at: clang/include/clang/Sema/Sema.h:512
+ static AlignPackInfo getFromRawEncoding(unsigned Encoding) {
+ static_assert(sizeof(AlignPackInfo) == sizeof(uint32_t),
+ "Size is not correct");
----------------
aaron.ballman wrote:
> I would feel more comfortable with this assertion if the class was using
> bit-fields to pack the values together. As it stands, we're kind of hoping
> that `bool`, `Mode`, and `unsigned char` will all pack in a particular way
> (and `bool`'s representation is implementation-defined).
Yeah, good point. I will move it back to previous bitfields version.
================
Comment at: clang/include/clang/Sema/Sema.h:527
+
+ unsigned char getPackNumber() const { return PackNumber; }
+
----------------
aaron.ballman wrote:
> Given that the ctor takes an `int` for this value, should this be returning
> an `int`?
As we know the pack number would not exceed 16 bytes, so the intention of using
`unsigned char` here is to save space taken by AlignPackInfo. And that's why I
added the diagnostics and assertion in the ctor to make sure the value won't be
truncated.
================
Comment at: clang/test/Sema/aix-pragma-pack-and-align.c:231
+
+// expected-no-warning
----------------
aaron.ballman wrote:
> Is this comment intentional?
Yes, my intention is to test no pragma pack or prama align is unterminated.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87702/new/
https://reviews.llvm.org/D87702
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
