[PATCH] D34114: [clang] A better format for unnecessary packed warning.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Some concrete suggestions throughout the patch, but I think we should take a step back and reconsider this warning approach: it seems bizarre for us to warn on any packed struct that happens to contain a `char`. It would make sense to warn if an `__attribute__((packed))` written in the source has *no* effect (because it's applied to a struct where all fields already have alignment 1, or because it's applied to a field that has alignment 1) -- even then I'm not entirely convinced this is a valuable warning, but I assume there's some reason you want to warn on it :)



================
Comment at: lib/AST/RecordLayoutBuilder.cpp:1894-1896
+      llvm::SmallString<128> WarningMsg = (UnnecessaryPackedFields.size() == 1)
+                                              ? StringRef("field:")
+                                              : StringRef("fields:");
----------------
We intend for our diagnostics to be translatable, so sentence fragments like this that assume a certain phrasing of an English-language diagnostic should not be hard-coded.

Instead, change the diagnostic to pass the number of fields and use %plural to select between the different word suffixes. See http://clang.llvm.org/docs/InternalsManual.html#formatting-a-diagnostic-argument for more information.


================
Comment at: lib/AST/RecordLayoutBuilder.cpp:1897-1900
+      for (const auto identifier : UnnecessaryPackedFields) {
+        WarningMsg += " '";
+        WarningMsg += identifier->getName();
+        WarningMsg += "',";
----------------
Rather than building a potentially very long list here, consider passing each field name to the diagnostic as a separate argument, and only list the first few of them, for example:

```
packed attribute is unnecessary for field 'i'
packed attribute is unnecessary for fields 'i' and 'j'
packed attribute is unnecessary for fields 'i', 'j', and 'k'
packed attribute is unnecessary for fields 'i', 'j', 'k', ...
```


================
Comment at: test/CodeGenCXX/warn-padded-packed.cpp:19
 
-struct S4 {
-  int i; // expected-warning {{packed attribute is unnecessary for 'i'}}
+struct S4 { // expected-warning {{packed attribute is unnecessary for field: 'i'}}
+  int i;
----------------
This looks backwards: the packed attribute *is* necessary for field `i` here (because it reduces `i`'s alignment from 4 to 1), but not for field `c`.


================
Comment at: test/CodeGenCXX/warn-padded-packed.cpp:49
 
 struct S9 { // expected-warning {{packed attribute is unnecessary for 'S9'}}
+  int x;
----------------
Again this seems wrong.


================
Comment at: test/CodeGenCXX/warn-padded-packed.cpp:75
 
+struct S14 { // expected-warning {{packed attribute is unnecessary for fields: 'i', 'j'}}
+  int i;
----------------
Again, this looks exactly backwards.


Repository:
  rL LLVM

https://reviews.llvm.org/D34114