[Sema] Suppress warnings for C's zero initializer

[sgilles]

Summary

Relax checks for -Wmissing-field-initializers and -Wmissing-braces so
that, for all C standards, assigning to a structure with { 0 } produces
no warnings. Add tests.

This fixes PR21689, which is mentioned by the Austin Group at
http://austingroupbugs.net/view.php?id=918 . It does not extend this
treatment of { 0 } to C++.

Patch by S. Gilles

[rsmith]

Why are you adding a language option for this? Just use !LangOpts.CPlusPlus.

[sgilles]

In D28148#632002, @rsmith wrote:

Why are you adding a language option for this? Just use !LangOpts.CPlusPlus.

I didn't want to have this change accidentally apply to other, non-C++ languages, since I'm not sure which languages would go through this path (Objective C, perhaps). If that's not a problem, I'll remove the language option and switch the checks to !LangOpts.CPlusPlus, which should be much cleaner.

[sgilles]

Instead of adding a language option to distinguish C, negatively check
against C++.

[rsmith]

lib/Sema/SemaInit.cpp

884 ↗

(On Diff #82638)

Remove empty line.

885–892 ↗

(On Diff #82638)

I think it would make more sense to check ParentIList here instead of StructuredSubobjectInitList -- we want to check whether the list the user wrote in the source code was {0}, not the list after semantic checking. This would matter for a case like:

struct A { int n; };
struct B { struct A a; };
struct C { struct B b; };
C c = {0};

ParentIList will be {0} at every level here, but it looks like StructuredSubobjectInitList will be {{0}} when checking the initialization of c.b, so you won't suppress the warning.

It would also matter for a case like

struct A { short p; };
struct B { A a; };
B b = {0};

where the list after semantic checking will have an implicit conversion wrapped around the initializer.

1843–1851 ↗

(On Diff #82638)

Please factor this check out into something like InitListExpr::isIdiomaticZeroInitializer(). It would make sense for that function to also assert isSyntactic().

[fhahn]

Ops, it looks like I accidentally switched the diff back to the initial version and I have no idea how to switch it back :(

I am very sorry about that, would you mind uploading the latest version again?

[• Quuxplusone]

Re the language option: In D27163, @rsmith wrote:

Perhaps some catch-all "I want defined behavior for things that C defines even though I'm compiling in C++" flag would make sense here?

It didn't happen then, but I'd like to re-raise the idea. There's probably plenty of code (e.g. Apple kernel drivers, judging from @rjmccall's comments on D27163) that's written in the common subset of C and C++ but compiled as C++. It's unfortunate that that workflow keeps getting "broken" by aggressive diagnostics (in this case) and aggressive optimizations (in that case). I'd like if Clang provided a way for the user to explicitly opt-in and say "Some or all of this code is C compiled as C++; please respect that."

(IOW, it looks as if this patch proposes to fix the noisy diagnostic for every language except for C-sometimes-compiled-as-C++, and as a sometime writer of C-sometimes-compiled-as-C++ myself, that irks me. Apologies if I've misconstrued the patch.)

[rjmccall]

In D28148#632541, @Quuxplusone wrote:

Re the language option: In D27163, @rsmith wrote:

Perhaps some catch-all "I want defined behavior for things that C defines even though I'm compiling in C++" flag would make sense here?

It didn't happen then, but I'd like to re-raise the idea. There's probably plenty of code (e.g. Apple kernel drivers, judging from @rjmccall's comments on D27163) that's written in the common subset of C and C++ but compiled as C++. It's unfortunate that that workflow keeps getting "broken" by aggressive diagnostics (in this case) and aggressive optimizations (in that case). I'd like if Clang provided a way for the user to explicitly opt-in and say "Some or all of this code is C compiled as C++; please respect that."

(IOW, it looks as if this patch proposes to fix the noisy diagnostic for every language except for C-sometimes-compiled-as-C++, and as a sometime writer of C-sometimes-compiled-as-C++ myself, that irks me. Apologies if I've misconstrued the patch.)

C++, for all its problems, does fix some things about C. I don't think it makes all that much sense to support a general "this is C being compiled as C++" mode, especially for diagnostics where (generally) compiling things as C++ ends up applying a more sensible language rule than C provides. We tend to have a lot more diagnostics that are basically pointing out flaws in C that C++ fixes than the other way around.

[sgilles]

Thank you for the comments, rsmith. I'm addressing them now, and I'll make sure to add your examples to the test case. I don't think isSyntactic() exists, so I'm using !getSyntactic() instead, which should have the desired effect.

In D28148#632484, @fhahn wrote:

Ops, it looks like I accidentally switched the diff back to the initial version and I have no idea how to switch it back :(

I am very sorry about that, would you mind uploading the latest version again?

No problem - I'll upload a new version to address rsmith's comments in a bit.

In D28148#632541, @Quuxplusone wrote:

(IOW, it looks as if this patch proposes to fix the noisy diagnostic for every language except for C-sometimes-compiled-as-C++, and as a sometime writer of C-sometimes-compiled-as-C++ myself, that irks me. Apologies if I've misconstrued the patch.)

My intention was only to fix the noisy diagnostic only for C compiled as C, because that's my particular use case and I didn't want to accidentally remove valuable warnings for other use cases. Extending this to the case of C-sometimes-compiled-as-C++ should be as easy as dropping the getLangOpts().CPlusPlus. But is this warning useful for C++-compiled-as-C++? Perhaps it would be better to wait until explicit recognition of C-as-C++ to do that.

[sgilles]

Address rsmith's comments, in particular: factor out testing zero initializers to a method of InitListExpr; use ParentIList instead of StructuredSubobjectInitList.

The warning is (still) not relaxed for C++ code. I have no opinion on this beyond wanting to avoid regressions, but for lack of consensus I'll default to changing as little as possible.

[danielmarjamaki]

Thanks for working on these. imo these are false positives.

lib/AST/Expr.cpp

1893 ↗

(On Diff #82720)

I would recommend capital first letter for this variable

1894 ↗

(On Diff #82720)

I suggest a single line:

return Lit && Lit->getValue() == 0;

[rsmith]

Looks good to go once Daniel's and my comments are addressed. Thank you.

lib/AST/Expr.cpp

1887 ↗

(On Diff #82720)

!isSyntacticForm() would be preferable here instead of !getSyntacticForm().

[sgilles]

Address danielmarjamaki's and rsmith's comments (creating InitListExpr::isSyntacticForm() since it didn't already exist), as well as correct syntax of test so that it actually runs.

[sgilles]

Thanks to danielmarjamaki and rsmith for comments, which I think this diff addresses. I have not done an extensive search of the codebase for places where isSyntacticForm() would be useful, but there don't seem to be any callers of getSyntacticForm() which used it in the way I did, so I believe that is okay.

lib/AST/Expr.cpp

1887 ↗

(On Diff #82720)

I believe isSyntacticForm() makes sense here - please correct me if I've misunderstood.

[sgilles]

Ping - if there are no comments, could this be accepted?

[danielmarjamaki]

LGTM

[danielmarjamaki]

This is not committed as far as I see.. do you have write permission or do you want that I commit it?

[sgilles]

Sure, if it still applies. I'm just a user and have no write access. Looks like I patched this locally and then forgot all about it.

cfe/trunk/include/clang/AST/Expr.h

public:
   // literal or an @encode?
   bool isStringLiteralInit() const;
 
   /// Is this a transparent initializer list (that is, an InitListExpr that is
   /// purely syntactic, and whose semantics are that of the sole contained
   /// initializer)?
   bool isTransparent() const;
 
+  /// Is this the zero initializer {0} in a language which considers it
+  /// idiomatic?
+  bool isIdiomaticZeroInitializer(const LangOptions &LangOpts) const;
+
   SourceLocation getLBraceLoc() const { return LBraceLoc; }
   void setLBraceLoc(SourceLocation Loc) { LBraceLoc = Loc; }
   SourceLocation getRBraceLoc() const { return RBraceLoc; }
   void setRBraceLoc(SourceLocation Loc) { RBraceLoc = Loc; }
 
   bool isSemanticForm() const { return AltForm.getInt(); }
   InitListExpr *getSemanticForm() const {
     return isSemanticForm() ? nullptr : AltForm.getPointer();
   }
+  bool isSyntacticForm() const {
+    return !AltForm.getInt() || !AltForm.getPointer();
+  }
   InitListExpr *getSyntacticForm() const {
     return isSemanticForm() ? AltForm.getPointer() : nullptr;
   }
 
   void setSyntacticForm(InitListExpr *Init) {
     AltForm.setPointer(Init);
     AltForm.setInt(true);
     Init->AltForm.setPointer(this);

cfe/trunk/lib/AST/Expr.cpp

bool InitListExpr::isTransparent() const {
   // transparent struct copy.
   if (!getInit(0)->isRValue() && getType()->isRecordType())
     return false;
 
   return getType().getCanonicalType() ==
          getInit(0)->getType().getCanonicalType();
 }
 
+bool InitListExpr::isIdiomaticZeroInitializer(const LangOptions &LangOpts) const {
+  assert(isSyntacticForm() && "only test syntactic form as zero initializer");
+
+  if (LangOpts.CPlusPlus || getNumInits() != 1) {
+    return false;
+  }
+
+  const IntegerLiteral *Lit = dyn_cast<IntegerLiteral>(getInit(0));
+  return Lit && Lit->getValue() == 0;
+}
+
 SourceLocation InitListExpr::getLocStart() const {
   if (InitListExpr *SyntacticForm = getSyntacticForm())
     return SyntacticForm->getLocStart();
   SourceLocation Beg = LBraceLoc;
   if (Beg.isInvalid()) {
     // Find the first non-null initializer.
     for (InitExprsTy::const_iterator I = InitExprs.begin(),
                                      E = InitExprs.end(); 

cfe/trunk/lib/Sema/SemaInit.cpp

if (!VerifyOnly) {
     if (EndIndex < ParentIList->getNumInits() &&
         ParentIList->getInit(EndIndex)) {
       SourceLocation EndLoc
         = ParentIList->getInit(EndIndex)->getSourceRange().getEnd();
       StructuredSubobjectInitList->setRBraceLoc(EndLoc);
     }
 
     // Complain about missing braces.
-    if (T->isArrayType() || T->isRecordType()) {
+    if ((T->isArrayType() || T->isRecordType()) &&
+        !ParentIList->isIdiomaticZeroInitializer(SemaRef.getLangOpts())) {
       SemaRef.Diag(StructuredSubobjectInitList->getLocStart(),
                    diag::warn_missing_braces)
           << StructuredSubobjectInitList->getSourceRange()
           << FixItHint::CreateInsertion(
                  StructuredSubobjectInitList->getLocStart(), "{")
           << FixItHint::CreateInsertion(
                  SemaRef.getLocForEndOfToken(
                      StructuredSubobjectInitList->getLocEnd()),
void InitListChecker::CheckStructUnionTypes(
   }
 
   // If structDecl is a forward declaration, this loop won't do
   // anything except look at designated initializers; That's okay,
   // because an error should get printed out elsewhere. It might be
   // worthwhile to skip over the rest of the initializer, though.
   RecordDecl *RD = DeclType->getAs<RecordType>()->getDecl();
   RecordDecl::field_iterator FieldEnd = RD->field_end();
-  bool CheckForMissingFields = true;
+  bool CheckForMissingFields =
+    !IList->isIdiomaticZeroInitializer(SemaRef.getLangOpts());
+
   while (Index < IList->getNumInits()) {
     Expr *Init = IList->getInit(Index);
 
     if (DesignatedInitExpr *DIE = dyn_cast<DesignatedInitExpr>(Init)) {
       // If we're not the subobject that matches up with the '{' for
       // the designator, we shouldn't be handling the
       // designator. Return immediately.
       if (!SubobjectIsDesignatorContext)

cfe/trunk/test/Sema/zero-initializer.c

+// RUN: %clang_cc1 -std=c99 -Wmissing-field-initializers -Wmissing-braces -verify %s
+
+// Tests that using {0} in struct initialization or assignment is supported
+struct foo { int x; int y; };
+struct bar { struct foo a; struct foo b; };
+struct A { int a; };
+struct B { struct A a; };
+struct C { struct B b; };
+
+int main(void)
+{
+  struct foo f = { 0 }; // no-warning
+  struct foo g = { 9 }; // expected-warning {{missing field 'y' initializer}}
+  struct foo h = { 9, 9 }; // no-warning
+  struct bar i = { 0 }; // no-warning
+  struct bar j = { 0, 0 }; // expected-warning {{suggest braces around initialization of subobject}} expected-warning {{missing field 'b' initializer}}
+  struct bar k = { { 9, 9 }, { 9, 9 } }; // no-warning
+  struct bar l = { { 9, 9 }, { 0 } }; // no-warning
+  struct bar m = { { 0 }, { 0 } }; // no-warning
+  struct bar n = { { 0 }, { 9, 9 } }; // no-warning
+  struct bar o = { { 9 }, { 9, 9 } }; // expected-warning {{missing field 'y' initializer}}
+  struct C p = { 0 }; // no-warning
+  struct C q = { 9 }; // expected-warning {{suggest braces around initialization of subobject}} expected-warning {{suggest braces around initialization of subobject}}
+  f = (struct foo ) { 0 }; // no-warning
+  g = (struct foo ) { 9 }; // expected-warning {{missing field 'y' initializer}}
+  h = (struct foo ) { 9, 9 }; // no-warning
+  i = (struct bar) { 0 }; // no-warning
+  j = (struct bar) { 0, 0 }; // expected-warning {{suggest braces around initialization of subobject}} expected-warning {{missing field 'b' initializer}}
+  k = (struct bar) { { 9, 9 }, { 9, 9 } }; // no-warning
+  l = (struct bar) { { 9, 9 }, { 0 } }; // no-warning
+  m = (struct bar) { { 0 }, { 0 } }; // no-warning
+  n = (struct bar) { { 0 }, { 9, 9 } }; // no-warning
+  o = (struct bar) { { 9 }, { 9, 9 } }; // expected-warning {{missing field 'y' initializer}}
+  p = (struct C) { 0 }; // no-warning
+  q = (struct C) { 9 }; // expected-warning {{suggest braces around initialization of subobject}} expected-warning {{suggest braces around initialization of subobject}}
+
+  return 0;
+}