Xiangling_L marked 9 inline comments as done.
Xiangling_L added inline comments.
================
Comment at: clang/include/clang/Sema/Sema.h:488
+ AlignPackInfo(AlignPackInfo::Mode M, int Num, bool IsAIX)
+ : PackAttr(true), AlignMode(M), PackNumber(Num), AIXStack(IsAIX) {}
+
----------------
jasonliu wrote:
> I noticed PackNumber is an unsigned char and we are passing an int type into
> it.
> Could we add an assertion in the constructor to make sure Num would never be
> something that's going to get truncated when it converts to an unsigned char?
I think the warning/error `expected #pragma pack parameter to be '1', '2', '4',
'8', or '16'` have already guaranteed that for us? Or maybe using `unsigned
int` makes people more comfortable?
================
Comment at: clang/include/clang/Sema/Sema.h:515
+ bool operator==(AlignPackInfo Info) const {
+ return AlignMode == Info.AlignMode && PackNumber == Info.PackNumber;
+ }
----------------
jasonliu wrote:
> This could return true when `PackAttr` in AlignPackInfo are not the same.
> Wouldn't that cause an issue?
(1) You mean we have two `AlignPackInfo` with same AlignMode and PackNumber,
but one is PackAttr and the other one is AlignAttr?
The example I can think of is:
```
a)#pragma align(packed)
#pragma pack(1) //AlignMode = Packed, PackNumber = 1
b) #pragma align(packed) //AlignMode = Packed, PackNumber = 1
```
But I don't think we have any issue in this case. Before and after my patch, a
== b.
Please let me know any other cases concerning you if any.
(2) However, your concerns leads me to think of another case, where behavior
changes with my patch.
```
a)
#pragma align(natural)
#pragma pack(1) /AlignMode = Native, PackNumber = 1
b)
#pragma align(packed) ///AlignMode = Packed, PackNumber = 1
```
Without this patch, a == b for other targets.
And I suppose a != b for AIX.
================
Comment at: clang/lib/Sema/SemaAttr.cpp:403
// Warn about modified alignment after #includes.
if (PrevPackState.CurrentValue != PackStack.CurrentValue) {
Diag(IncludeLoc, diag::warn_pragma_pack_modified_after_include);
----------------
jasonliu wrote:
> Since we changed the PackStack for it to contain AlignPackInfo instead of
> unsigned.
> This stack no longer only contains Pack information. So we need to rethink
> about how this diagnostic and the one follows should work.
> i.e. What's the purpose of these diagnostic? Is it still only for pragma pack
> report? If so, what we are doing here is not correct, since the
> `CurrentValue` could be different, not because of the pragma pack change, but
> because of the pragma align change.
> If it's not only for pragma pack any more, but also intend to detect the
> pragma align interaction, then possibly function name and diagnostic needs
> some modification, as they don't match the intent any more.
Thanks for pointing this out. I agree that what we are doing here is not
correct.
The original commit[45b40147117668ce65bff4f6a240bdae4ad4bf7d] message shows:
```
This commit adds a new -Wpragma-pack warning. It warns in the following
cases:
- When a translation unit is missing terminating #pragma pack (pop)
directives.
- When entering an included file if the current alignment value as
determined
by '#pragma pack' directives is different from the default alignment
value.
- When leaving an included file that changed the state of the current
alignment
value.
```
So it looks these warnings are used only for `pragma pack`.
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
