[PATCH] D131084: Add support for specifying the severity of a SARIF Result.

[Vaibhav Yenamandra via Phabricator via cfe-commits]

vaibhav.y marked an inline comment as not done.
vaibhav.y added inline comments.


================
Comment at: clang/include/clang/Basic/Sarif.h:157
+/// 1. <a href="https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317648">level property</a>
+enum class SarifResultLevel { Note, Warning, Error };
+
----------------
aaron.ballman wrote:
> Should this include `Remark` for diagnostics like: https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/DiagnosticFrontendKinds.td#L55
> 
> If not, I think the comments should explain how to map remarks to one of these levels.
Ack, if we're adding `remark`, should I also add support the "informational"/"fail" result kind that was previously discussed by you? Currently there's no `kind`, so we are defaulting to "fail".


================
Comment at: clang/lib/Basic/Sarif.cpp:25
 #include "llvm/Support/ConvertUTF.h"
+#include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/JSON.h"
----------------
aaron.ballman wrote:
> Why was this include required?
It provides `llvm_unreachable`. so far it was available transitively, must have been added when I implemented `resultLevelToStr(...)`.

I don't feel strongly about keeping this though.


================
Comment at: clang/lib/Basic/Sarif.cpp:396-399
+  if (Result.LevelOverride.hasValue())
+    Ret["level"] = resultLevelToStr(Result.LevelOverride.getValue());
+  else
+    Ret["level"] = resultLevelToStr(Rule.DefaultConfiguration.Level);
----------------
aaron.ballman wrote:
> (Probably have to re-run clang-format)
Good catch! I notice Optional<T> has `value_or`, is that okay as well here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131084