cjdb added a subscriber: Naghasan. cjdb added a comment. In D131084#3699223 <https://reviews.llvm.org/D131084#3699223>, @aaron.ballman wrote:
> In D131084#3697678 <https://reviews.llvm.org/D131084#3697678>, @cjdb wrote: > >> It seems I got confused and conflated the two this morning. >> >> In D131084#3697525 <https://reviews.llvm.org/D131084#3697525>, @vaibhav.y >> wrote: >> >>>> Hmm, we can probably use "informational" for notes, warnings, and remarks, >>>> but I'm kinda partial to proposing the latter two upstream. >>> >>> Interesting, I agree that remarks could fit under "informational". >>> >>> As for notes: As I understand it they are always child diagnostics of >>> warnings/errors, so seems it would be okay to associate these with the >>> "fail" kind. >> >> I don't think classifying warnings as "fail" is right unless `-Werror` is >> being used. For notes, I think there may be two options, if we don't want to >> use informational: >> >> - inherit from the parent diagnostic >> - use notApplicable, since they don't have any influence on their own >> >> >> >> In D131084#3697308 <https://reviews.llvm.org/D131084#3697308>, @denik wrote: >> >>> In D131084#3697256 <https://reviews.llvm.org/D131084#3697256>, @cjdb wrote: >>> >>>> In D131084#3697211 <https://reviews.llvm.org/D131084#3697211>, @vaibhav.y >>>> wrote: >>>> >>>>> Submitting for review: >>>>> >>>>> Some notes: >>>>> >>>>> There are a couple of ways I think we can acheive this, per the spec: >>>>> >>>>> 1. The reportingDescriptor (rule) objects can be given a default >>>>> configuration property >>>>> <https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317850>, >>>>> which can set the default warning level and other data such as rule >>>>> parameters etc. >>>>> 2. The reportingDescriptor objects can omit the default configuration >>>>> (which then allows operating with warning as default), and the level is >>>>> then set when the result is reported. >>>>> >>>>> The first approach would be "more correct", what are your thoughts on >>>>> this? Would we benefit from having per-diagnostic configuration? >>>>> >>>>> There is also the question about the "kind" of results in clang. From my >>>>> reading of the spec >>>>> <https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317647>, >>>>> it seems that "fail" is the only case that applies to us because: >>>>> >>>>> - "pass": Implies no issue was found. >>>>> - "open": This value is used by proof-based tools. It could also mean >>>>> additional assertions required >>>>> - "informational": The specified rule was evaluated and produced a purely >>>>> informational result that does not indicate the presence of a problem >>>>> - "notApplicable": The rule specified by ruleId was not evaluated, >>>>> because it does not apply to the analysis target. >>>>> >>>>> Of these "open" and "notApplicable" seem to be ones that *could* come to >>>>> use but I'm not sure where / what kind of diagnostics would use these. >>>>> Probably clang-tidy's `bugprone-*` suite? >>>>> >>>>> Let me know what you think is a good way to approach this wrt clang's >>>>> diagnostics system. >>>> >>>> Hmm, we can probably use "informational" for notes, warnings, and remarks, >>>> but I'm kinda partial to proposing the latter two upstream. >>> >>> I think we can skip `kind` in the clang diagnostic. I found this: >>> >>>> If kind is absent, it SHALL default to "fail". >>>> If level has any value other than "none" and kind is present, then kind >>>> SHALL have the value "fail". >> >> If skipping the kind defaults to fail, then we shouldn't skip it, because >> getting a warning or remark doesn't necessarily mean that the build has >> failed. > > My suggestion is that we only need to worry about two kinds: `fail` for > warnings and errors and `informational` for remarks. When the kind if `fail`, > I'd expect we'd use the level `warning` or `error`. > > Notes are interesting though. Sometimes they're extra information about the > diagnostic on the given line, so I was kind of expecting they'd be specified > via `attachments` in that case. But they can also sometimes be used to > represent code flow (like in some CFG based warnings where we're showing > where something was called from), and they can also sometimes be a related > location (like "declared here" notes). Perhaps we may want to expose those to > SARIF in different ways depending on the situation? I proposed relaxing the kind upstream last night. <https://github.com/oasis-tcs/sarif-spec/issues/539>, since I think that it might be good to discern when a warning isn't an error with `warning`/`informational`, because then consumers could trivially treat them differently. I think `review` is the best one for remarks, because some of them do communicate potential problems (`remark_fe_backend_optimization_remark_analysis_aliasing` is the only public one I can see that's like this, but I do recall a few from ComputeCpp too (cc @Naghasan). Perhaps these ones should instead be promoted to warnings?) Notes that are exposed as SARIF could be `none`/`informational`, while remarks are `none`/`review`? A part of my endgame is to see notes be incorporated into their parents, but that's a long way off methinks. 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
