[PATCH] D58291: [clangd] Include textual diagnostic ID as Diagnostic.code.

[Sam McCall via Phabricator via cfe-commits]

sammccall marked an inline comment as done.
sammccall added a comment.

In D58291#1400569 <https://reviews.llvm.org/D58291#1400569>, @kadircet wrote:

> LG but is this information really useful to users? According to LSP `The diagnostic's code, which might appear in the user interface.`, I think seeing this will be mostly noise for users.


It's a good question, it depends how this is surfaced, and we may want to tweak the behavior or suppress entirely in some cases.
I think at least some are useful:

- clang-tidy check names are things users need to know about (used for configuration)
- for warnings, we quite likely should replace with the most specific warning category (e.g. "unreachable-code-loop-increment"), again these are used for configuration (-W)
- for others, maybe we should at least trim the err_ prefix, or maybe drop them entirely.



================
Comment at: clangd/Diagnostics.cpp:39
+#include "clang/Basic/DiagnosticCommentKinds.inc"
+#include "clang/Basic/DiagnosticSemaKinds.inc"
+#include "clang/Basic/DiagnosticAnalysisKinds.inc"
----------------
kadircet wrote:
> I suppose `CrossTUKinds` is left out intentionally ?
Yeah, this isn't really part of clang, and seems to be part of static analyzer.
Some places include it and others don't...


================
Comment at: clangd/Diagnostics.cpp:281
+    if (auto* Name = getDiagnosticCode(D.ID))
+      Main.code = Name;
     if (Opts.EmbedFixesInDiagnostics) {
----------------
jkorous wrote:
> It seems to me that in case `ID` is undefined (hits the default case in `getDiagnosticCode`) we are calling `std::string` non-explicit constructor with `nullptr` which is UB.
> How about changing `char* getDiagnosticCode` to `Optional<char*> getDiagnosticCode` or similar to make it less error-prone?
This is an if statement - if the pointer is null we never assign to std::string.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58291/new/

https://reviews.llvm.org/D58291