[PATCH] D71963: clang-tidy doc: Add the severities description

[Kim Viggedal via Phabricator via cfe-commits]

vingeldal added a comment.

It's very hard to write these kinds of definitions without ambiguity and plenty of subjective interpretation creeping in. I'll try my best to provide constructive feedback but I'm admittedly struggling a bit with providing helpful counter proposals.
Ideally these levels would be based on a data driven approach, like when we say that something is error prone we should be able to support that with data.

I think the high severity is well defined it's distinctly different from the other levels from a qualitative perspective.
I would also like to suggest the addition that any check reporting security vulnerabilities should be classified as high severity.
One way to make all levels qualitatively different would be to use something like the following definitions:

- Medium:  things that aren't direct a error but are error prone somehow (ideally there would be data to support the claim that this issue often cause errors)
- Low: things that are not direct errors neither, prone to indirectly cause errors, but which still cause quality issues like unnecessarily low performance.
- Style: things that are handled by clang-format rather than clang-tidy.

Given how hard it is to write these kinds of definitions clearly I'm not sure it is a good idea to introduce severity for clang tidy checks.
Unless we can find a very strong definitions for distinctly different levels, supported with actual data, it might be better to just not do it.
Also I think there is already a difference in severity level indicated by whether the check is part of clang-format, clang-tidy or clang-diagnostics
and by defining these severity levels we need to make sure they don't conflict in any way with these already implied severity levels.

In short: My first suggestion is to just remove severity from the table instead of trying to improve the definitions.
If there is a strong preference towards keeping them I suggest making them more qualitatively different as described above.



================
Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:414
+
+- **STYLE**: A true positive indicates that the source code is against a specific coding guideline or could improve readability.
+  Example: LLVM Coding Guideline: Do not use else or else if after something that interrupts control flow (break, return, throw, continue).
----------------
It seems like there is a bit of overlap between STYLE and LOW. They both mention readability.
Might I suggest that style could be only issues related to naming convention and text formatting, like indentation, line breaks -like things that could be clang format rules.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:418
+- **LOW**: A true positive indicates that the source code is hard to read/understand or could be easily optimized.
+  Example: Unused variables, Dead code.
+
----------------
To me it's not obvious why these examples aren't medium severity. They seem to fit the description of medium severity, they are also very similar to the example in medium severity.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:423
+
+- **HIGH**: A true positive indicates that the source code will cause a run-time error.
+  Example of this category: out of bounds array access, division by zero, memory leak.
----------------
Does something need to always result in division by zero to be of high severity or is it enough that it introduces the possibility for division by zero to occur?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71963