Page MenuHomePhabricator

[Sema] Suppress additional warnings for C's zero initializer
ClosedPublic

Authored by al3xtjames on May 12 2019, 2:02 PM.

Details

Summary

D28148 relaxed some checks for assigning { 0 } to a structure for all C standards, but it failed to handle structures with non-integer subobjects. Relax -Wmissing-braces checks for such structures, and add some additional tests.

This fixes PR39931.

Diff Detail

Repository
rL LLVM

Event Timeline

al3xtjames created this revision.May 12 2019, 2:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 12 2019, 2:02 PM
al3xtjames edited the summary of this revision. (Show Details)

Formatting changes.

al3xtjames edited the summary of this revision. (Show Details)May 12 2019, 4:28 PM

This looks reasonable to fix the problem at hand, but would it handle nested structures too?

struct s1 {
   short f1;  // "int f1" is fine.
};
struct s2 {
   struct s1 f2;
   int x;
};
struct s3 {
   struct s2 f3;
   int x;
};
struct s2 x1 = {0};
struct s3 x2 = {0};

produces this AST (clang-check -ast-dump x.c --):

|-VarDecl 0x55e7bbc60390 <line:13:1, col:18> col:11 x1 'struct s2':'struct s2' cinit
| `-InitListExpr 0x55e7bbc60498 <col:16, col:18> 'struct s2':'struct s2'
|   |-InitListExpr 0x55e7bbc604e8 <col:17> 'struct s1':'struct s1'
|   | `-ImplicitCastExpr 0x55e7bbc60530 <col:17> 'short' <IntegralCast>
|   |   `-IntegerLiteral 0x55e7bbc60430 <col:17> 'int' 0
|   `-ImplicitValueInitExpr 0x55e7bbc60548 <<invalid sloc>> 'int'
|-VarDecl 0x55e7bbc605b0 <line:14:1, col:18> col:11 x2 'struct s3':'struct s3' cinit
| `-InitListExpr 0x55e7bbc60678 <col:16, col:18> 'struct s3':'struct s3'
|   |-InitListExpr 0x55e7bbc606c8 <col:17> 'struct s2':'struct s2'
|   | |-InitListExpr 0x55e7bbc60718 <col:17> 'struct s1':'struct s1'
|   | | `-ImplicitCastExpr 0x55e7bbc60760 <col:17> 'short' <IntegralCast>
|   | |   `-IntegerLiteral 0x55e7bbc60610 <col:17> 'int' 0
|   | `-ImplicitValueInitExpr 0x55e7bbc60778 <<invalid sloc>> 'int'
|   `-ImplicitValueInitExpr 0x55e7bbc60788 <<invalid sloc>> 'int'

Such nested structures work fine (no warnings generated).

Lekensteyn accepted this revision.May 16 2019, 5:25 PM

Thanks, I have also verified this patch against the above test case.

This revision is now accepted and ready to land.May 16 2019, 5:25 PM

Thanks for reviewing! I don't have commit access, so I can't commit this patch myself.

rsmith added a subscriber: rsmith.May 18 2019, 3:55 PM
rsmith added inline comments.
clang/lib/AST/Expr.cpp
2092–2096 ↗(On Diff #199192)

Use Expr::IgnoreImplicit rather than checking for an ImplicitCastExpr here:

const IntegerLiteral *Lit = dyn_cast<IntegerLiteral>(getInit(0)->IgnoreImplicit());
al3xtjames updated this revision to Diff 200159.EditedMay 18 2019, 5:01 PM

Switched to using Expr::IgnoreImplicit() instead of manually checking for an ImplicitCastExpr.

Lekensteyn accepted this revision.May 20 2019, 10:00 AM

Verified again!

@rsmith Are you happy with the changes, is it ready to be merged?

@rsmith The latest patch version has addressed your feedback, can you confirm that this is ready to be merged?

@al3xtjames I was about to commit this but noticed that some others check whether getInit(0) is NULL or not before proceeding. Should that be done here as well? If not, why?

See for example "Harden InitListExpr::isStringLiteralInit() against getInit() returning null." in
https://github.com/llvm/llvm-project/commit/256bd96d1c2464b0f0ecb482ca52075a3158f6a1#diff-dac09655ff6a54658c320a28a6ea297c
and InitListExpr::isTransparent in
https://github.com/llvm/llvm-project/commit/122f88d481971b68d05ba12395c3b73ab4b31ab3#diff-dac09655ff6a54658c320a28a6ea297c

al3xtjames marked an inline comment as done.

Added null check for getInit(0)

Lekensteyn accepted this revision.Jul 15 2019, 5:08 PM

Thanks, I'll push once the build and test pass.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2019, 6:13 PM