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

Authored by yamaguchi on Apr 28 2017, 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.Apr 28 2017, 7:59 AM
ruiu added inline comments.Apr 28 2017, 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.Apr 28 2017, 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.Apr 30 2017, 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.Apr 30 2017, 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.Apr 30 2017, 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.Apr 30 2017, 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.Apr 30 2017, 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.Apr 30 2017, 6:11 AM
yamaguchi marked 5 inline comments as done.

Changed to early return.

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

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

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

Please use clang-format to format your code.

thakis added inline comments.May 1 2017, 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.May 1 2017, 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.May 1 2017, 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.

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?