Page MenuHomePhabricator

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

Authored by sgilles on Dec 28 2016, 4:28 PM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

sgilles updated this revision to Diff 82626.Dec 28 2016, 4:28 PM
sgilles retitled this revision from to [Sema] Suppress warnings for C's zero initializer.
sgilles updated this object.
sgilles added reviewers: cfe-commits, rsmith, zaks.anna.
rsmith edited edge metadata.Dec 28 2016, 4:50 PM

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

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 updated this revision to Diff 82638.Dec 28 2016, 5:26 PM
sgilles updated this object.
sgilles edited edge metadata.

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

zaks.anna edited reviewers, added: arphaman; removed: zaks.anna.Dec 28 2016, 9:33 PM
zaks.anna added a subscriber: dexonsmith.
rsmith added inline comments.Dec 29 2016, 12:48 PM
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 updated this revision to Diff 82626.Dec 29 2016, 2:17 PM
fhahn added a subscriber: fhahn.
fhahn added a comment.Dec 29 2016, 2:26 PM

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?

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

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.

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.

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.

(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 updated this revision to Diff 82720.Dec 29 2016, 10:23 PM

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.

sgilles marked 3 inline comments as done.Dec 29 2016, 10:25 PM

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 added a comment.Jan 2 2017, 1:01 PM

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 updated this revision to Diff 82849.EditedJan 3 2017, 1:40 AM
sgilles marked 2 inline comments as done.

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 marked an inline comment as done.Jan 3 2017, 1:44 AM

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 marked an inline comment as done.Jan 10 2017, 11:12 AM

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

This revision is now accepted and ready to land.Jan 11 2017, 2:14 AM

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

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.

This revision was automatically updated to reflect the committed changes.