[PATCH] D81785: [clangd] Fix readability-else-after-return 'Adding a note without main diagnostic' crash

[Haojian Wu via Phabricator via cfe-commits]

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