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

Authored by yamaguchi on Fri, Apr 28, 7:59 AM.

Details

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

Diff Detail

yamaguchi created this revision.Fri, Apr 28, 7:59 AM
ruiu added inline comments.Fri, Apr 28, 8:13 AM
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 updated this revision to Diff 97182.Fri, Apr 28, 10:38 PM

Update testcase. Made it minimal.

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 marked 2 inline comments as done.Sun, Apr 30, 4:30 AM
yamaguchi added inline comments.
lib/Sema/SemaInit.cpp
892

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

v.g.vassilev added inline comments.Sun, Apr 30, 4:35 AM
lib/Sema/SemaInit.cpp
892

I'd rewrite this using early returns:

if (SpellingLoc.isInvalid() || SemaRef.getSourceManager().isInSystemHeader(SpellingLoc))
  return;
...
yamaguchi added inline comments.Sun, Apr 30, 4:37 AM
lib/Sema/SemaInit.cpp
892

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

yamaguchi added inline comments.Sun, Apr 30, 4:38 AM
lib/Sema/SemaInit.cpp
892

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

yamaguchi updated this revision to Diff 97224.Sun, Apr 30, 5:42 AM

@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.

yamaguchi updated this revision to Diff 97227.Sun, Apr 30, 6:11 AM
yamaguchi marked 5 inline comments as done.

Changed to early return.

yamaguchi updated this revision to Diff 97228.Sun, Apr 30, 6:31 AM

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

ruiu added inline comments.Sun, Apr 30, 6:37 AM
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 added inline comments.Sun, Apr 30, 6:39 AM
lib/Sema/SemaInit.cpp
897

Please use clang-format to format your code.

thakis added inline comments.Mon, May 1, 2:41 PM
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 updated this revision to Diff 97398.Mon, May 1, 11:19 PM
yamaguchi marked 2 inline comments as done.

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

yamaguchi added inline comments.Mon, May 1, 11:29 PM
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.