[PATCH] D20561: Warn when taking address of packed member

[Aaron Ballman via cfe-commits]

aaron.ballman added a comment.

This is getting really close, I mostly only have nits left.


================
Comment at: lib/Sema/SemaExpr.cpp:10529
@@ +10528,3 @@
+    bool ByteAligned = Context.getTypeAlignInChars(MD->getType()).isOne();
+    if (ByteAligned) // packed has had not any effect on it
+      break;
----------------
Comments should be complete sentences including capitalization and punctuation, so perhaps instead: `// Packed does not have any effect.`

================
Comment at: test/Sema/address-packed.c:21
@@ +20,3 @@
+
+extern void f3(void());
+
----------------
This is unused and can be removed.

================
Comment at: test/Sema/address-packed.c:27
@@ +26,3 @@
+
+void g0() {
+  {
----------------
Function should take `(void)` since this is C.

================
Comment at: test/Sema/address-packed.c:47
@@ +46,3 @@
+    f2(&arguable.c0); // no-warning
+    f1(&arguable.x);  // expected-warning {{packed member 'x' of class or structure }}
+    f2(&arguable.c1); // no-warning
----------------
Extra space before `}}`.

================
Comment at: test/Sema/address-packed.c:51
@@ +50,3 @@
+  {
+    struct Arguable *arguable = get_arguable();
+    f2(&(arguable->c0)); // no-warning
----------------
Should put in a comment explaining that this produces no diagnostic because of the parens.

================
Comment at: test/SemaCXX/address-packed.cpp:13
@@ +12,3 @@
+
+typedef struct Arguable ArguableT;
+
----------------
This is unused and can be removed.

================
Comment at: test/SemaCXX/address-packed.cpp:77
@@ +76,2 @@
+  }
+};
----------------
Can we get one further test?
```
template <typename Ty>
class __attribute__((packed)) S {
  Ty X;
public:
  Ty *get() const {
    return &X; // no warning?
  }
};
```
I am fine if this does not warn. I am also fine if it only warns when there are instantiations of S for which Ty should warn (e.g., do an explicit instantiation with `char` that does not warn and another one with `int` that does).


http://reviews.llvm.org/D20561