[PATCH] D87702: [Frontend] Add pragma align natural and sort out pragma pack stack effect

[Jason Liu via Phabricator via cfe-commits]

jasonliu accepted this revision.
jasonliu added a comment.

LGTM with minor nits.



================
Comment at: clang/lib/Sema/SemaAttr.cpp:74
     return;
-  // The #pragma align/pack affected a record in an included file, so Clang
-  // should warn when that the pragma was written in a file that included the
+  // The #pragma align/pack affected a record in an included file,  so Clang
+  // should warn when that pragma was written in a file that included the
----------------



================
Comment at: clang/lib/Sema/SemaAttr.cpp:347-349
+    AlignmentVal = CurVal.getPackNumber();
+    if (!CurVal.IsPackSet())
       AlignmentVal = 8;
----------------
nit


================
Comment at: clang/lib/Sema/SemaAttr.cpp:372
+  // XL pragma pack does not support identifier syntax.
+  if (IsXLPragma && !SlotLabel.empty()) {
+    Diag(PragmaLoc, diag::err_pragma_pack_identifer_not_supported);
----------------
Could we move this to the top of the function so that we could have a quick exit when we sees this?
If we are going to emit an error, the `AlignPackStack.Act(PragmaLoc, Action, StringRef(), Info);` is not necessary any more.


================
Comment at: clang/lib/Sema/SemaAttr.cpp:555
+      });
+      // If we found the label so pop from there.
+      if (I != Stack.rend()) {
----------------



================
Comment at: clang/lib/Sema/SemaAttr.cpp:581
+    } else if (!Stack.empty()) {
+      // xl '#pragma align' sets the base line,
+      // and pragma pack cannot pop over the base line.
----------------



================
Comment at: clang/lib/Sema/SemaAttr.cpp:582
+      // xl '#pragma align' sets the base line,
+      // and pragma pack cannot pop over the base line.
+      if (Value.IsXLStack() && Value.IsPackAttr() && CurrentValue.IsAlignAttr())
----------------



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87702/new/

https://reviews.llvm.org/D87702