Skip to content

Conversation

hstk30-hw
Copy link
Contributor

@hstk30-hw hstk30-hw commented Dec 20, 2023

#73077 added -Wswitch-default diagnostic but it produced false positives in templates. This PR will address that. #75943

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Dec 20, 2023
@llvmbot
Copy link
Member

llvmbot commented Dec 20, 2023

@llvm/pr-subscribers-clang

Author: None (hstk30-hw)

Changes

Fix #75943


Full diff: https://github.com/llvm/llvm-project/pull/76007.diff

2 Files Affected:

  • (modified) clang/lib/Sema/SemaStmt.cpp (+1-3)
  • (added) clang/test/Sema/switch-default-template.cpp (+27)
diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp
index 63348d27a8c94a..adc2055ec4e659 100644
--- a/clang/lib/Sema/SemaStmt.cpp
+++ b/clang/lib/Sema/SemaStmt.cpp
@@ -1327,9 +1327,6 @@ Sema::ActOnFinishSwitchStmt(SourceLocation SwitchLoc, Stmt *Switch,
     }
   }
 
-  if (!TheDefaultStmt)
-    Diag(SwitchLoc, diag::warn_switch_default);
-
   if (!HasDependentValue) {
     // If we don't have a default statement, check whether the
     // condition is constant.
@@ -1344,6 +1341,7 @@ Sema::ActOnFinishSwitchStmt(SourceLocation SwitchLoc, Stmt *Switch,
       assert(!HasConstantCond ||
              (ConstantCondValue.getBitWidth() == CondWidth &&
               ConstantCondValue.isSigned() == CondIsSigned));
+      Diag(SwitchLoc, diag::warn_switch_default);
     }
     bool ShouldCheckConstantCond = HasConstantCond;
 
diff --git a/clang/test/Sema/switch-default-template.cpp b/clang/test/Sema/switch-default-template.cpp
new file mode 100644
index 00000000000000..c671164bd785b0
--- /dev/null
+++ b/clang/test/Sema/switch-default-template.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wswitch-default %s
+
+template<typename Index>
+int f1(Index i)
+{
+  switch (i) {              // expected-warning {{'switch' missing 'default' label}}
+    case 0: return 0;
+    case 1: return 1;
+  }
+  return 0;
+}
+
+template<typename Index>
+int f2(Index i)
+{
+  switch (i) {            // no-warning
+    case 0: return 0;
+    case 1: return 1;
+    default: return 2;
+  }
+  return 0;
+}
+
+int main() {
+  return f1(1);       // expected-note {{in instantiation of function template specialization 'f1<int>' requested here}}
+}
+

@shafik
Copy link
Collaborator

shafik commented Dec 20, 2023

Can you add more details to you summary e.g. "#73077 added -Wswitch-default diagnostic but it produced false positives in templates. This PR will address that issue"

@hstk30-hw hstk30-hw force-pushed the fix-Wswitch-default-template branch from c3d5ac4 to 7b8c1c7 Compare December 20, 2023 06:36
@dongjianqiang2
Copy link
Contributor

thanks for correcting this.

@hstk30-hw hstk30-hw force-pushed the fix-Wswitch-default-template branch from 7b8c1c7 to 2991e9b Compare December 20, 2023 07:32
Copy link
Contributor

@cor3ntin cor3ntin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks

Copy link
Member

@aaronpuchert aaronpuchert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now I guess this is Ok, although I think the better fix would be to diagnose missing or duplicate default labels even in the dependent case. Because default labels themselves are never dependent.

@hstk30-hw hstk30-hw requested a review from shafik December 21, 2023 00:55
@shafik
Copy link
Collaborator

shafik commented Dec 21, 2023

For now I guess this is Ok, although I think the better fix would be to diagnose missing or duplicate default labels even in the dependent case. Because default labels themselves are never dependent.

We should probably add a FIXME for this.

Copy link
Contributor

@dongjianqiang2 dongjianqiang2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks

@hstk30-hw hstk30-hw force-pushed the fix-Wswitch-default-template branch from 2991e9b to d6c5cfe Compare December 21, 2023 12:29
Copy link

github-actions bot commented Dec 21, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

[llvm#73077] added Wswitch-default diagnostic but it produced false
positives in templates. This PR address it.
@hstk30-hw hstk30-hw force-pushed the fix-Wswitch-default-template branch from d6c5cfe to 8e00f93 Compare December 21, 2023 13:41
@hstk30-hw hstk30-hw merged commit 033ec09 into llvm:main Dec 22, 2023
searlmc1 pushed a commit to ROCm/llvm-project that referenced this pull request Jan 13, 2024
This test case exists upstream currently. It was deleted by
mistake when reapplying the patch:
 [clang][Sema] Add -Wswitch-default warning option (llvm#73077)

The test case had been added upstream for a subsequent patch
 [Clang][Sema] Fix Wswitch-default bad warning in template (llvm#76007)

Change-Id: I94fdbe2daa4703166e0d7fc00a3a4d08f98ae410
searlmc1 pushed a commit to ROCm/llvm-project that referenced this pull request Oct 28, 2024
cherry-picked:
9e1f1cf sdkrystian@gmail.com      Tue Jul  9 16:40:53 2024 -0400 [Clang][Sema] Handle class member access expressions with valid nested-name-specifiers that become invalid after lookup (llvm#98167)
584e431 sdkrystian@gmail.com      Wed Jul  3 12:12:53 2024 -0400 [Clang][Sema] Treat explicit specializations of static data member templates declared without 'static' as static data members when diagnosing uses of 'auto' (llvm#97425)
a1bce0b dblaikie@gmail.com        Thu Jun 27 08:17:40 2024 -0700 Clang: Add warning flag for storage class specifiers on explicit specializations (llvm#96699)
f46d146 erickvelez7@gmail.com     Fri May 31 11:02:21 2024 -0700 [clang] require template arg list after template kw (llvm#80801)
033ec09 hanwei62@huawei.com       Fri Dec 22 09:00:41 2023 +0800 [Clang][Sema] Fix Wswitch-default bad warning in template (llvm#76007)
c281782 dongjianqiang2@huawei.com Thu Dec  7 09:03:15 2023 +0800 [clang][Sema] Add -Wswitch-default warning option (llvm#73077)

Change-Id: Ib2582201b01cc62c3bf62011347925556e8531f7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants