njames93 created this revision.
njames93 added reviewers: sammccall, hokein.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, jkorous,
MaskRay, ilya-biryukov.
Herald added a project: clang.
njames93 added a comment.
The actual fix in `ElseAfterReturnCheck.cpp` is needed.
However clangd's handling of Remarks is a little suspicious. Remarks are
supposed to be different to notes in the sense they are designed to be stand
alone, unlike notes which depend on a another diagnostic. see Add 'remark'
diagnostic type in 'clang'
<https://github.com/llvm/llvm-project/commit/741602461d2079c682916bce6701c39acb08bbd3>
This seems to be how clangd determines what a note is:
bool isNote(DiagnosticsEngine::Level L) {
return L == DiagnosticsEngine::Note || L == DiagnosticsEngine::Remark;
}
Fix a crash in clangd caused by an (admittidly incorrect) Remark diagnositic
being emitted from readability-else-after-return.
This crash doesn't occur in clang-tidy so there are no tests there for this.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D81785
Files:
clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -299,6 +299,25 @@
DiagSource(Diag::ClangTidy), DiagName("modernize-loop-convert"))));
}
+TEST(DiagnosticTest, NoAddingANoteWithoutMainDiagnostic) {
+ Annotations Main(R"cc(
+ void foo() {
+ if (int X = 0) {
+ return;
+ } [[else]] {
+ X++;
+ }
+ }
+ )cc");
+ TestTU TU = TestTU::withCode(Main.code());
+ TU.ClangTidyChecks = "readability-else-after-return";
+ EXPECT_THAT(TU.build().getDiagnostics(),
+ UnorderedElementsAre(::testing::AllOf(
+ Diag(Main.range(), "do not use 'else' after 'return'"),
+ DiagSource(Diag::ClangTidy),
+ DiagName("readability-else-after-return"))));
+}
+
TEST(DiagnosticTest, ClangTidySuppressionComment) {
Annotations Main(R"cpp(
int main() {
Index: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
@@ -188,9 +188,8 @@
if (IsLastInScope) {
// If the if statement is the last statement its enclosing statements
// scope, we can pull the decl out of the if statement.
- DiagnosticBuilder Diag =
- diag(ElseLoc, WarningMessage, clang::DiagnosticIDs::Level::Remark)
- << ControlFlowInterruptor;
+ DiagnosticBuilder Diag = diag(ElseLoc, WarningMessage)
+ << ControlFlowInterruptor;
if (checkInitDeclUsageInElse(If) != nullptr) {
Diag << tooling::fixit::createReplacement(
SourceRange(If->getIfLoc()),
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -299,6 +299,25 @@
DiagSource(Diag::ClangTidy), DiagName("modernize-loop-convert"))));
}
+TEST(DiagnosticTest, NoAddingANoteWithoutMainDiagnostic) {
+ Annotations Main(R"cc(
+ void foo() {
+ if (int X = 0) {
+ return;
+ } [[else]] {
+ X++;
+ }
+ }
+ )cc");
+ TestTU TU = TestTU::withCode(Main.code());
+ TU.ClangTidyChecks = "readability-else-after-return";
+ EXPECT_THAT(TU.build().getDiagnostics(),
+ UnorderedElementsAre(::testing::AllOf(
+ Diag(Main.range(), "do not use 'else' after 'return'"),
+ DiagSource(Diag::ClangTidy),
+ DiagName("readability-else-after-return"))));
+}
+
TEST(DiagnosticTest, ClangTidySuppressionComment) {
Annotations Main(R"cpp(
int main() {
Index: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
@@ -188,9 +188,8 @@
if (IsLastInScope) {
// If the if statement is the last statement its enclosing statements
// scope, we can pull the decl out of the if statement.
- DiagnosticBuilder Diag =
- diag(ElseLoc, WarningMessage, clang::DiagnosticIDs::Level::Remark)
- << ControlFlowInterruptor;
+ DiagnosticBuilder Diag = diag(ElseLoc, WarningMessage)
+ << ControlFlowInterruptor;
if (checkInitDeclUsageInElse(If) != nullptr) {
Diag << tooling::fixit::createReplacement(
SourceRange(If->getIfLoc()),
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
