Page MenuHomePhabricator

[Sema] Fix -Wuninitialized for struct assignment from GNU C statement expression
ClosedPublic

Authored by Nathan-Huckleberry on Jul 12 2019, 4:18 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2019, 4:19 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Thanks for the patch. Minor nits looking for test cases that should have no warning (like the two examples provided in the bug).

clang/test/Sema/warn-uninitialized-statement-expression.c
7 ↗(On Diff #209637)

Can you please add a case like foo without z in it (so that no warnings are expected, as in pr42604)? (You can keep both test cases, just looking for 1 more, as the first case).

20 ↗(On Diff #209637)

Ditto about adding a clear case that should not warn.

21 ↗(On Diff #209637)

This needs a trailing comment like:

int x = my_widget.x; // fixme: we should probably warn about this case

and file a bug about it. Doesn't need to be solved here.

  • Add warning-free test cases
clang/lib/Sema/SemaDecl.cpp
10890 ↗(On Diff #209644)
clang/test/Sema/warn-uninitialized-statement-expression.c
35 ↗(On Diff #209644)

Can you please file a bug pointing to this test case?

Nathan-Huckleberry marked 3 inline comments as done.
  • Change cast type
Nathan-Huckleberry marked 3 inline comments as done.Jul 15 2019, 10:40 AM
Nathan-Huckleberry added inline comments.
clang/test/Sema/warn-uninitialized-statement-expression.c
21 ↗(On Diff #209637)
rsmith added a comment.EditedJul 17 2019, 7:40 PM

I don't think this problem really has anything to do with statement expressions; consider:

struct Widget a = (init2(&a), a);

... which has the same behaviour and presumably produces the same warning.

It's just a C / C++ difference. In C++, these examples are undefined because the lifetime of the object hasn't started yet, but I think in C they're valid. We should just disable the whole warning in C mode and leave it to the CFG analysis.

  • Disable self reference checking for C
nickdesaulniers accepted this revision.Tue, Jul 23, 3:29 PM

Thanks for the patch and following up on the review.

This revision is now accepted and ready to land.Tue, Jul 23, 3:29 PM
aaron.ballman accepted this revision.Wed, Jul 24, 5:15 AM

LGTM aside from a comment request.

clang/lib/Sema/SemaDecl.cpp
11258 ↗(On Diff #211230)

This should probably have a comment explaining why we only do it in C++ mode.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFri, Jul 26, 10:29 AM