[PATCH] D28231: -Wunreachable-code: Avoid multiple diagnostics that are triggered by the same source range and fix the unary operator fixit source range

[Alex Lorenz via Phabricator via cfe-commits]

arphaman created this revision.
arphaman added reviewers: rsmith, bruno, ahatanak.
arphaman added a subscriber: cfe-commits.
arphaman set the repository for this revision to rL LLVM.

This patch fixes an issue with -Wunreachable-code diagnostic that happens with the following code sample:

  struct s {
    void *p;
  };
  
  void fn(struct s *s) {
    if (1 || !s->p)
      return;
    fn(s);
  }

Currently, clang will emit two warnings (at `fn(s)` and at `!s->p`. As well as that, the fixits for the first warning (for `fn(s)`) has an incorrect source range: it suggests to wrap `!s->p` in parentheses when it should suggest to warp `1` instead.

The attached patch ensures that the fixits for the `fn(s)` warning suggest to wrap the correct expression (`1`). It also avoids the second warning that was previously shown for `!s->p` because that warning is caused by the same expression (`1`).

Thanks for taking a look.


Repository:
  rL LLVM

https://reviews.llvm.org/D28231

Files:
  lib/Analysis/ReachableCode.cpp
  lib/Sema/AnalysisBasedWarnings.cpp
  test/Sema/warn-unreachable.c


Index: test/Sema/warn-unreachable.c
===================================================================
--- test/Sema/warn-unreachable.c
+++ test/Sema/warn-unreachable.c
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 %s -fsyntax-only -verify -fblocks -Wunreachable-code-aggressive -Wno-unused-value -Wno-covered-switch-default -I %S/Inputs
+// RUN: %clang_cc1 -fsyntax-only -fblocks -Wunreachable-code-aggressive -Wno-unused-value -Wno-covered-switch-default -fdiagnostics-parseable-fixits -I %S/Inputs %s 2>&1 | FileCheck %s
 
 #include "warn-unreachable.h"
 
@@ -396,3 +397,30 @@
   else
     calledFun();
 }
+
+// rdar://24570531
+
+struct StructWithPointer {
+  void *p;
+};
+
+void emitJustOneWarningForOr(struct StructWithPointer *s) {
+  if (1 || !s->p) // expected-note {{silence by adding parentheses to mark code as explicitly dead}}
+    return; // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:"/* DISABLES CODE */ ("
+            // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:8-[[@LINE-2]]:8}:")"
+  emitJustOneWarningForOr(s); // expected-warning {{code will never be executed}}
+}
+
+void emitJustOneWarningForOrSilenced(struct StructWithPointer *s) {
+  if ((1) || !s->p)
+    return;
+
+  emitJustOneWarningForOrSilenced(s); // no warning
+}
+
+void emitJustOneWarningForOr2(struct StructWithPointer *s) {
+  if (1 || !s->p) // expected-warning {{code will never be executed}}
+    return; // expected-note at -1 {{silence by adding parentheses to mark code as explicitly dead}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:7-[[@LINE-2]]:7}:"/* DISABLES CODE */ ("
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:8-[[@LINE-3]]:8}:")"
+}
Index: lib/Sema/AnalysisBasedWarnings.cpp
===================================================================
--- lib/Sema/AnalysisBasedWarnings.cpp
+++ lib/Sema/AnalysisBasedWarnings.cpp
@@ -56,14 +56,24 @@
 namespace {
   class UnreachableCodeHandler : public reachable_code::Callback {
     Sema &S;
+    SourceRange PreviousSilenceableCondVal;
+
   public:
     UnreachableCodeHandler(Sema &s) : S(s) {}
 
     void HandleUnreachable(reachable_code::UnreachableKind UK,
                            SourceLocation L,
                            SourceRange SilenceableCondVal,
                            SourceRange R1,
                            SourceRange R2) override {
+      // Avoid reporting multiple unreachable code diagnostics that are
+      // triggered by the same conditional value.
+      if (PreviousSilenceableCondVal.isValid() &&
+          SilenceableCondVal.isValid() &&
+          PreviousSilenceableCondVal == SilenceableCondVal)
+        return;
+      PreviousSilenceableCondVal = SilenceableCondVal;
+
       unsigned diag = diag::warn_unreachable;
       switch (UK) {
         case reachable_code::UK_Break:
Index: lib/Analysis/ReachableCode.cpp
===================================================================
--- lib/Analysis/ReachableCode.cpp
+++ lib/Analysis/ReachableCode.cpp
@@ -218,8 +218,8 @@
     }
     case Stmt::UnaryOperatorClass: {
       const UnaryOperator *UO = cast<UnaryOperator>(S);
-      if (SilenceableCondVal) 
-        *SilenceableCondVal = UO->getSourceRange();      
+      if (SilenceableCondVal && !SilenceableCondVal->getBegin().isValid())
+        *SilenceableCondVal = UO->getSourceRange();
       return UO->getOpcode() == UO_LNot &&
              isConfigurationValue(UO->getSubExpr(), PP, SilenceableCondVal,
                                   IncludeIntegers, WrappedInParens);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D28231.82873.patch
Type: text/x-patch
Size: 3481 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170103/cfe69f18/attachment.bin>