hokein added a comment. Thanks for spotting it!
In D81785#2091352 <https://reviews.llvm.org/D81785#2091352>, @njames93 wrote: > The actual fix in `ElseAfterReturnCheck.cpp` is needed. yeah, the fix in clang-tidy check seems reasonable to me, this aligns the general practice. In general, we use clang::warning to emit clang-tidy check warnings. > 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; > } yeah, I think you're right, we should fix that in clangd as well. We might not hit this in practice -- clangd runs syntax-only action, and `Remark` diagnostics seems to be emitted from clang optimization phrase (which is not executed in clangd). ================ Comment at: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp:191 // 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; ---------------- this would change the output of the check, I suppose this is not covered in `readability-else-after-return.cpp` lit test (that test only tests `warning` messages), could you add a test there? then we don't need a test case in clangd. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81785/new/ https://reviews.llvm.org/D81785 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
