Tweak how -Wunused-value interacts with macros

[thakis]

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

[thakis]

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

[thakis]

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

[rnk]

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.

[thakis]

Done and landed in r251441, thanks!

lib/Sema/SemaStmt.cpp

void Sema::DiagnoseUnusedExprResult(const Stmt *S) {
   if (!E)
     return;
 
   // If we are in an unevaluated expression context, then there can be no unused
   // results because the results aren't expected to be used in the first place.
   if (isUnevaluatedContext())
     return;
 
-  SourceLocation ExprLoc = E->IgnoreParens()->getExprLoc();
+  SourceLocation ExprLoc = E->IgnoreParenImpCasts()->getExprLoc();
   // In most cases, we don't want to warn if the expression is written in a
   // macro body, or if the macro comes from a system header. If the offending
   // expression is a call to a function with the warn_unused_result attribute,
   // we warn no matter the location. Because of the order in which the various
   // checks need to happen, we factor out the macro-related test here.
   bool ShouldSuppress = 
       SourceMgr.isMacroBodyExpansion(ExprLoc) ||
       SourceMgr.isInSystemMacro(ExprLoc);
 
   const Expr *WarnExpr;
   SourceLocation Loc;
   SourceRange R1, R2;
   if (!E->isUnusedResultAWarning(WarnExpr, Loc, R1, R2, Context))
     return;
 
   // If this is a GNU statement expression expanded from a macro, it is probably
   // unused because it is a function-like macro that can be used as either an
   // expression or statement.  Don't warn, because it is almost certainly a
   // false positive.
   if (isa<StmtExpr>(E) && Loc.isMacroID())
     return;
 
+  // Check if this is the UNREFERENCED_PARAMETER from the Microsoft headers.
+  // That macro is frequently used to suppress "unused parameter" warnings,
+  // but its implementation makes clang's -Wunused-value fire.  Prevent this.
+  if (getLangOpts().MSVCCompat && isa<ParenExpr>(E->IgnoreImpCasts()) &&
+      Loc.isMacroID()) {
+    SourceLocation SpellLoc = Loc;
+    if (findMacroSpelling(SpellLoc, "UNREFERENCED_PARAMETER"))
+      return;
+  }
+
   // Okay, we have an unused result.  Depending on what the base expression is,
   // we might want to make a more specific diagnostic.  Check for one of these
   // cases now.
   unsigned DiagID = diag::warn_unused_expr;
   if (const ExprWithCleanups *Temps = dyn_cast<ExprWithCleanups>(E))
     E = Temps->getSubExpr();
   if (const CXXBindTemporaryExpr *TempExpr = dyn_cast<CXXBindTemporaryExpr>(E))
     E = TempExpr->getSubExpr();

test/Sema/unused-expr.c

 // RUN: %clang_cc1 -fsyntax-only -verify %s -Wno-unreachable-code
+// RUN: %clang_cc1 -DMS_COMPAT -fms-compatibility -fsyntax-only -verify %s -Wno-unreachable-code
 
 int foo(int X, int Y);
 
 double sqrt(double X);  // implicitly const because of no -fmath-errno!
 
 void bar(volatile int *VP, int *P, int A,
          _Complex double C, volatile _Complex double VC) {
   

 #undef NOP
 #undef M1
 #undef M2
 #undef M3
 #undef M4
 #undef M5
 #undef M6
 #undef M7
+
+#define UNREFERENCED_PARAMETER(x) (x)
+
+void unused_parm(int a) {
+#ifdef MS_COMPAT
+  UNREFERENCED_PARAMETER(a);
+#else
+  UNREFERENCED_PARAMETER(a); // expected-warning {{expression result unused}}
+#endif
+}