[PATCH] D35484: Add a warning for missing '#pragma pack (pop)' and suspicious '#pragma pack' uses when including files

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:716
+def warn_pragma_pack_non_default_at_include : Warning<
+  "non-default #pragma pack value might change the alignment of records in the "
+  "included file">,
----------------
We don't use "record" in the text for diagnostics. Perhaps replace record with "struct or union members"?


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:718
+  "included file">,
+  InGroup<PragmaPack>;
+def warn_pragma_pack_modified_after_include : Warning<
----------------
This can move up a line.


================
Comment at: include/clang/Sema/Sema.h:1162
 
+  sema::SemaPPCallbacks *SemaPPCallbackHandler;
+
----------------
Please group this with the other private Sema members and give it a \brief comment.


================
Comment at: lib/Sema/Sema.cpp:95
+      if (IncludeLoc.isValid()) {
+        // Entering an include.
+        IncludeStack.push_back(IncludeLoc);
----------------
This comment doesn't add much value.


================
Comment at: lib/Sema/SemaAttr.cpp:235-236
+    return;
+  for (auto I = PackStack.Stack.rbegin(), E = PackStack.Stack.rend(); I != E;
+       ++I)
+    Diag(I->PragmaPushLocation, diag::warn_pragma_pack_no_pop_eof);
----------------
Can you use `for (const auto &StackSlot : llvm::reverse(PackStack.Stack))` instead?


================
Comment at: lib/Sema/SemaAttr.cpp:287-288
   if (Action & PSK_Push)
-    Stack.push_back(Slot(StackSlotLabel, CurrentValue, CurrentPragmaLocation));
+    Stack.push_back(Slot(StackSlotLabel, CurrentValue, CurrentPragmaLocation,
+                         PragmaLocation));
   else if (Action & PSK_Pop) {
----------------
This would be better written using `emplace_back()` rather than constructing and using `push_back()`.


Repository:
  rL LLVM

https://reviews.llvm.org/D35484