Page MenuHomePhabricator

[Sema] -Wcomma should not warn for expressions that return void
AbandonedPublic

Authored by arphaman on Jun 29 2017, 4:24 AM.

Details

Summary

Right now -Wcomma is too strict IMO, we shouldn't warn about expressions that return void.

Diff Detail

Repository
rL LLVM

Event Timeline

arphaman created this revision.Jun 29 2017, 4:24 AM
arphaman retitled this revision from [Sema] -Wcomma should not warn for expression that return void to [Sema] -Wcomma should not warn for expressions that return void.
rnk edited edge metadata.Jun 29 2017, 10:53 AM

I thought the intention of -Wcomma was to warn on practically all non-macro uses of the comma operator. I know it's silly to cast void to void, but I seem to recall that this was an intentional style-ish warning like -Wparentheses, which encourages if ((x = y)) for intentional assignments in ifs.

I thought void-returning functions were supposed to be allowed based on the description in https://reviews.llvm.org/D3976 , but later in that discussion the definition was changed to instead allow almost nothing.

rtrieu edited edge metadata.Jun 29 2017, 3:06 PM

Reid is correct, the whitelisted expressions was greatly reduced during code review so only casts to void would disable the warning. While the last review did not have the description updated to reflect this, the committed code does have an accurate description. What is the reason to exclude void expressions now? For the function case, it is more consistent to warn on all function calls since we can't determine if a function returns void just by looking at the call site.

Ah, I see, so this is more of a stylistic warning rather than "suspicious" use one.

What is the reason to exclude void expressions now?  For the function case, it is more consistent to warn on all function calls since we can't determine if a function returns void just by looking at the call site.

We've had an issue with -Wcomma adoption internally and have thought that this was a bug.

arphaman abandoned this revision.Jul 3 2017, 1:37 AM

Abandoning. The current behaviour makes sense. Thanks for the responses!