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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
