Page MenuHomePhabricator

Tweak how -Wunused-value interacts with macros
ClosedPublic

Authored by thakis on Oct 21 2015, 7:47 PM.

Details

Reviewers
rnk
rsmith
Summary
  1. Make the warning more strict in C mode. r172696 added code to suppress warnings from macro expansions in system headers, which checks SourceMgr.isMacroBodyExpansion(E->IgnoreParens()->getExprLoc()). Consider this snippet:
#define FOO(x) (x)
void f(int a) {
  FOO(a);
}
 In C, the line `FOO(a)` is an `ImplicitCastExpr(ParenExpr(DeclRefExpr))`, while it's just a `ParenExpr(DeclRefExpr)` in C++. So in C++, `E->IgnoreParens()` returns the `DeclRefExpr` and the check tests the SourceLoc of `a`. In C, the `ImplicitCastExpr` has the effect of checking the SourceLoc of `FOO`, which is a macro body expansion, which causes the diagnostic to be skipped. It looks unintentional that clang does different things for C and C++ here, so use `IgnoreParenImpCasts` instead of `IgnoreParens` here. This has the effect of the warning firing more often than previously in C code – it now fires as often as it fires in C++ code.

2. In MS mode, suppress the warning if it would warn on `UNREFERENCED_PARAMETER`. `UNREFERENCED_PARAMETER` is a commonly used macro on Windows and it happens to uselessly trigger -Wunused-value. As discussed in the thread "rfc: winnt.h's UNREFERENCED_PARAMETER() vs clang's -Wunused-value" on cfe-dev, fix this by special-casing this specific macro. (This costs a string comparison and some fast-path lexing per warning, but the warning is emitted rarely. It fires once in Windows.h itself, so this code runs at least once per TU including Windows.h, but it doesn't run hundreds of times.)

Diff Detail

Event Timeline

thakis updated this revision to Diff 38075.Oct 21 2015, 7:47 PM
thakis retitled this revision from to Tweak how -Wunused-value interacts with macros.
thakis updated this object.
thakis added a reviewer: rsmith.
thakis updated this object.
thakis updated this object.
thakis updated this object.
thakis added a subscriber: cfe-commits.

Sorry about all the emails, I didn't realize that editing the description would send an update every time. Looks like phab's markup language doesn't support code blocks in lists (https://secure.phabricator.com/T4939). The description looks ok in email but broken on phab. Read the change description in your mail client :-)

rnk: Can you look at this if Richard is still away doing committee thing?

rnk accepted this revision.Oct 27 2015, 12:39 PM
rnk edited edge metadata.

lgtm, I don't feel strongly about whether -fms-compatibility matters, but I'd prefer that it didn't.

lib/Sema/SemaStmt.cpp
224

I'd rather not make this conditional on -fms-compatibility. I can easily imagine a situation where someone has cargo-culted this pattern from windows.h while porting to other platforms, and silencing the unused value warning in that situation seems like goodness. I guess the only concern is lexer performance, but I think it'll be fine.

This revision is now accepted and ready to land.Oct 27 2015, 12:39 PM
thakis closed this revision.Oct 27 2015, 12:50 PM
thakis marked an inline comment as done.

Done and landed in r251441, thanks!