Fix a bug that -Wmissing-braces fires on system headers

[yamaguchi]

Summary

This is an update patch for bug [1].

-Wmissing-braces should not fire for system headers.

[1] https://bugs.llvm.org/show_bug.cgi?id=24007

[ruiu]

test/Sema/warn-missing-braces.c

6–11

You can simplify this, can't you? It seems something like

struct foo { unsigned data[2]; };

suffices.

19

So this can be something like struct foo b = BAR;

[yamaguchi]

Update testcase. Made it minimal.

[v.g.vassilev]

LGTM, modulo the comment.

lib/Sema/SemaInit.cpp

892

I'd avoid double negations. Could you use isInvalid instead of !isValid. That would make the condition more readable.

[yamaguchi]

lib/Sema/SemaInit.cpp

892

It's not (!SpellingLoc.isValid()) but is !((SpellingLoc.isValid() && SemaRef.getSourceManager().isInSystemHeader(SpellingLoc))

[v.g.vassilev]

lib/Sema/SemaInit.cpp

892

I'd rewrite this using early returns:

if (SpellingLoc.isInvalid() || SemaRef.getSourceManager().isInSystemHeader(SpellingLoc))
  return;
...

[yamaguchi]

lib/Sema/SemaInit.cpp

892

Do you think I should change this to (SpellingLoc.isInvalid() || !(SemaRef.getSourceManager().isInSystemHeader(SpellingLoc)) ??

[yamaguchi]

lib/Sema/SemaInit.cpp

892

Oh, I see.
Sorry I didn't see your reply because I was typing mine.

[yamaguchi]

@v.g.vassilev
I don't think early return is good idea, because someone might want to add some code under this function in the future.

[v.g.vassilev]

I don't think we should worry about that. Please also see: http://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code

[yamaguchi]

Changed to early return.

[v.g.vassilev]

Did you incorporate the comment from here: https://bugs.llvm.org/show_bug.cgi?id=24007#c2

[yamaguchi]

Add check if there is -Wmissing-braces enabled or not.

[ruiu]

lib/Sema/SemaInit.cpp

875

Is it intentional that you run the new code only when !VerifyOnly?

890–891

This piece of code seems a bit puzzling as you initialize a variable to some value and immediately re-assign to some other value. It's better to initialize it only once.

SourceLocation SpellingLoc = SemaRef.getSourceManager().getSpellingLoc(
  StructuredSubobjectInitList->getLocStart());

[ruiu]

lib/Sema/SemaInit.cpp

897

Please use clang-format to format your code.

[thakis]

test/Sema/warn-missing-braces.c

21

This looks shorter than the test I pasted on the bug. Does it get the corner case right that my patch on that bug didn't get right?

[yamaguchi]

Fixed spaces and changed SpellingLoc from two lines to one line.

[yamaguchi]

lib/Sema/SemaInit.cpp

875

I think VerifyOnly is used to check if it is formal verification mode or not, and if VerifyOnly == 1, it is supposed to emit no diagnostics. So I think it is OK that this code is here, because this part of is code emits -Wmissing-braces if braces are not appropriate.

[teemperor]

Everything beside this last test case seems to be handled. And from what I remember there was a longer discussion how to properly handle this case and so this review got stuck.

Can we add this last test case with a FIXME and then get this merged?

lib/Sema/SemaInit.cpp

 
   // Check the element types and build the structural subobject.
   unsigned StartIndex = Index;
   CheckListElementTypes(Entity, ParentIList, T,
                         /*SubobjectIsDesignatorContext=*/false, Index,
                         StructuredSubobjectInitList,
                         StructuredSubobjectInitIndex);
 
   if (!VerifyOnly) {
     StructuredSubobjectInitList->setType(T);
 
     unsigned EndIndex = (Index == StartIndex? StartIndex : Index - 1);
     // Update the structured sub-object initializer so that it's ending
     // range corresponds with the end of the last initializer it used.
     if (EndIndex < ParentIList->getNumInits() &&
         ParentIList->getInit(EndIndex)) {
       SourceLocation EndLoc
         = ParentIList->getInit(EndIndex)->getSourceRange().getEnd();
       StructuredSubobjectInitList->setRBraceLoc(EndLoc);
     }
 
-    // Complain about missing braces.
+    // Complain about missing braces when rhs is not a macro from system header.
     if (T->isArrayType() || T->isRecordType()) {
-      SemaRef.Diag(StructuredSubobjectInitList->getLocStart(),
-                   diag::warn_missing_braces)
-          << StructuredSubobjectInitList->getSourceRange()
-          << FixItHint::CreateInsertion(
-                 StructuredSubobjectInitList->getLocStart(), "{")
-          << FixItHint::CreateInsertion(
-                 SemaRef.getLocForEndOfToken(
-                     StructuredSubobjectInitList->getLocEnd()),
-                 "}");
+      SourceLocation SpellingLoc = StructuredSubobjectInitList->getLocStart();
+      SpellingLoc = SemaRef.getSourceManager().getSpellingLoc(SpellingLoc);
+      if (SpellingLoc.isInvalid() || 
+            !(SemaRef.getSourceManager().isInSystemHeader(SpellingLoc))) {
+        SemaRef.Diag(StructuredSubobjectInitList->getLocStart(),
+                     diag::warn_missing_braces)
+            << StructuredSubobjectInitList->getSourceRange()
+            << FixItHint::CreateInsertion(
+                   StructuredSubobjectInitList->getLocStart(), "{")
+            << FixItHint::CreateInsertion(
+                   SemaRef.getLocForEndOfToken(
+                       StructuredSubobjectInitList->getLocEnd()),
+                   "}");
+      }
     }
   }
 }
 
 /// Warn that \p Entity was of scalar type and was initialized by a
 /// single-element braced initializer list.
 static void warnBracedScalarInit(Sema &S, const InitializedEntity &Entity,
                                  SourceRange Braces) {

test/Sema/warn-missing-braces.c

 // RUN: %clang_cc1 -fsyntax-only -Wmissing-braces -verify %s
 
+#ifdef BE_THE_HEADER
+#pragma clang system_header
+
+typedef struct _foo {
+    unsigned char Data[2];
+} foo;
+
+#define BAR { 0 }
+
+#else
+
+#define BE_THE_HEADER
+#include __FILE__
+
 int a[2][2] = { 0, 1, 2, 3 }; // expected-warning{{suggest braces}} expected-warning{{suggest braces}}
+
+foo g = BAR; // should not show warnings
+
+#endif