Page MenuHomePhabricator

[clangd] Include textual diagnostic ID as Diagnostic.code.
ClosedPublic

Authored by sammccall on Feb 15 2019, 10:39 AM.

Diff Detail

Event Timeline

sammccall created this revision.Feb 15 2019, 10:39 AM
kadircet accepted this revision.Feb 17 2019, 6:50 AM

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.

clangd/Diagnostics.cpp
40

I suppose CrossTUKinds is left out intentionally ?

This revision is now accepted and ready to land.Feb 17 2019, 6:50 AM
jkorous requested changes to this revision.Feb 18 2019, 1:34 PM

Hi Sam, this looks good!
I found just one small detail.

clangd/Diagnostics.cpp
282

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 revision now requires changes to proceed.Feb 18 2019, 1:34 PM
sammccall updated this revision to Diff 195522.Apr 17 2019, 2:14 AM
sammccall marked an inline comment as done.

Rebase to head and expand scope a bit:

  • now also setting code for clang-tidy checks
  • to enable this to be used from the C++ API, the string code is now on Diag
  • and also expose Source over LSP (just an oversight?)
sammccall marked an inline comment as done.Apr 17 2019, 2:21 AM

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.
clangd/Diagnostics.cpp
40

Yeah, this isn't really part of clang, and seems to be part of static analyzer.
Some places include it and others don't...

282

This is an if statement - if the pointer is null we never assign to std::string.

kadircet accepted this revision.Apr 17 2019, 2:53 AM

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.

I see, I believe we can decide on what tweaks to perform after landing this patch and seeing behaviors in different editors, but up to you.

As for the lit-tests, it would be great to have a diag with source clang-tidy if it is not too much of a hustle.

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.

I see, I believe we can decide on what tweaks to perform after landing this patch and seeing behaviors in different editors, but up to you.

As for the lit-tests, it would be great to have a diag with source clang-tidy if it is not too much of a hustle.

Done.
There are some things I know I want to tweak (clang-tidy keeps the name of the check in the message...), will follow up.

This revision was not accepted when it landed; it landed in state Needs Review.Apr 17 2019, 5:34 AM
This revision was automatically updated to reflect the committed changes.