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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
