aaron.ballman added inline comments.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:858
+def err_pragma_pack_invalid_alignment : Error<
+ "expected #pragma pack parameter to be '1', '2', '4', '8', or '16'">;
def warn_pragma_pack_non_default_at_include : Warning<
----------------
================
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>;
----------------
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?
================
Comment at: clang/include/clang/Sema/Sema.h:486
+ : PackAttr(true), AlignMode(M), PackNumber(Num), XLStack(IsXL) {
+ assert(Num == PackNumber && "Unexpected value.");
+ }
----------------
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`.
================
Comment at: clang/include/clang/Sema/Sema.h:494
+
+ AlignPackInfo(bool IsXL) : AlignPackInfo(Native, IsXL) {}
+
----------------
================
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");
----------------
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).
================
Comment at: clang/include/clang/Sema/Sema.h:527
+
+ unsigned char getPackNumber() const { return PackNumber; }
+
----------------
Given that the ctor takes an `int` for this value, should this be returning an
`int`?
================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1314
void ItaniumRecordLayoutBuilder::InitializeLayout(const Decl *D) {
+
if (const RecordDecl *RD = dyn_cast<RecordDecl>(D)) {
----------------
Unintended whitespace change?
================
Comment at: clang/lib/Sema/SemaAttr.cpp:69-71
+ if (IsXLPragma && M == AlignPackInfo::Natural) {
+ RD->addAttr(AlignNaturalAttr::CreateImplicit(Context));
}
----------------
================
Comment at: clang/test/Sema/aix-pragma-pack-and-align.c:231
+
+// expected-no-warning
----------------
Is this comment intentional?
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
