aaron.ballman 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>;
----------------
Xiangling_L wrote:
> 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?
I'm not super familiar with this feature, so what I'm thinking of may not be a
real issue in practice. The case I was thinking of is where the user does
`#pragma pack(push, r1, 2)` to push a packing by name followed by `#pragma
pack(push, r2, 4)`, and the later pops come in the reverse order (`#pragma
pack(pop, r1)` followed by `#pragma pack(pop, r2)`). The first pop will
actually pop the `r2` packing and not the `r1` packing, which could cause the
field layout to be different from what the user expected.
================
Comment at: clang/include/clang/Sema/Sema.h:524-525
+ return AlignPackInfo(M, PackNumber, IsXL);
+ else
+ return AlignPackInfo(M, IsXL);
+ }
----------------
No `else` after a `return`.
================
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");
----------------
Xiangling_L wrote:
> 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.
Oh, that's different from what I was expecting -- I was thinking you'd change
the data member fields of the class to be bit-fields.
Either way works for me, though, so no change necessary unless you prefer that
approach.
================
Comment at: clang/include/clang/Sema/Sema.h:527
+
+ unsigned char getPackNumber() const { return PackNumber; }
+
----------------
Xiangling_L wrote:
> 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.
We don't save any space with this function signature though -- the return value
will most likely wind up being implicitly promoted to `int` anyway. I think the
fact that we store it as a smaller datatype internally is an implementation
detail and as far as the consumer of this class is concerned, the pack number
is an integer type where we don't care about the size. tbh, because the pack
numbers are always positive, I'd have the ctor and this function both deal with
an `unsigned` rather than an `int`.
(Note, I'm not talking about the type of the underlying field -- I think it's
fine to continue to use a smaller datatype there for space savings. I'm only
talking about the types in the public interface.)
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
