jasonliu added inline comments.
================
Comment at: clang/include/clang/Sema/Sema.h:66
#include "llvm/Frontend/OpenMP/OMPConstants.h"
+#include <cmath>
#include <deque>
----------------
Do we need cmath?
================
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) {}
+
----------------
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?
================
Comment at: clang/include/clang/Sema/Sema.h:504
+ unsigned getPackNumber() const { return PackNumber; }
+ void setPackNumber(int Num) { PackNumber = Num; }
+
----------------
I don't see this function gets referenced in this patch.
================
Comment at: clang/include/clang/Sema/Sema.h:514
+
+ bool operator==(AlignPackInfo Info) const {
+ return AlignMode == Info.AlignMode && PackNumber == Info.PackNumber;
----------------
I think a normal operator== signature would look like this:
bool operator==(const AlignPackInfo &Info) const;
================
Comment at: clang/include/clang/Sema/Sema.h:515
+ bool operator==(AlignPackInfo Info) const {
+ return AlignMode == Info.AlignMode && PackNumber == Info.PackNumber;
+ }
----------------
This could return true when `PackAttr` in AlignPackInfo are not the same.
Wouldn't that cause an issue?
================
Comment at: clang/include/clang/Sema/Sema.h:519
+ bool operator!=(AlignPackInfo Info) const {
+ return AlignMode != Info.AlignMode || PackNumber != Info.PackNumber;
+ }
----------------
You should be able to implement this using "operator==" right?
e.g. return !(*this == Info);
================
Comment at: clang/include/clang/Sema/Sema.h:524
+ /// \brief True if this is a pragma pack attribute,
+ // not a pragma align attribute.
+ bool PackAttr;
----------------
================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:697
InferAlignment(false), Packed(false), IsUnion(false),
- IsMac68kAlign(false), IsMsStruct(false), UnfilledBitsInLastUnit(0),
- LastBitfieldTypeSize(0), MaxFieldAlignment(CharUnits::Zero()),
- DataSize(0), NonVirtualSize(CharUnits::Zero()),
+ IsMac68kAlign(false), IsNaturalAlign(false), IsMsStruct(false),
+ UnfilledBitsInLastUnit(0), LastBitfieldTypeSize(0),
----------------
This could probably confuse people, as natural alignment is the default on most
target except 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);
----------------
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.
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
