donat.nagy added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:301-302
+ pluralSuffix(MaximalAllowedShift));
+ R->addNote(LeftNote, PathDiagnosticLocation{LHS, Ctx.getSourceManager(),
+ Ctx.getLocationContext()});
+ Ctx.emitReport(std::move(R));
----------------
gamesh411 wrote:
> donat.nagy wrote:
> > NoQ wrote:
> > > Can we just append this to the warning? The `addNote()` is useful for
> > > notes that need to be placed in code outside of the execution path, but
> > > if it's always next to the warning, it probably doesn't make sense to
> > > display it separately.
> > I didn't append this to the warning because I felt that the warning text is
> > already too long. (By the way, an earlier internal variant of this checker
> > produced two separate notes next to the warning message.)
> >
> > I tweaked the messages of this checker before initiating this review, but I
> > feel that there's still some room for improvements. (E.g. in this
> > particular case perhaps we could omit some of the details that are not
> > immediately useful and could be manually calculated from other parts of the
> > message...)
> Just a thought on simplifying the diagnostics a bit:
>
> Warning: "Right operand is negative in left shift"
> Note: "The result of left shift is undefined because the right operand is
> negative"
> Shortened: "Undefined left shift due to negative right operand"
>
> Warning: "Left shift by '34' overflows the capacity of 'int'"
> Note: "The result of left shift is undefined because the right operand '34'
> is not smaller than 32, the capacity of 'int'"
> Shortened: "Undefined left shift: '34' exceeds 'int' capacity (32 bits)"
>
> The more complex notes are a bit sketchy, as relevant information can be
> lost, and the following solution is probably a bit too much, but could prove
> to be an inspiration:
>
> Warning: "Left shift of '1024' overflows the capacity of 'int'"
> CXX Note: "Left shift of '1024' is undefined because 'int' can hold only 32
> bits (including the sign bit), so some bits overflow"
> CXX Note: "The value '1024' is represented by 11 bits, allowing at most 21
> bits for bitshift"
> C Note: "Left shift of '1024' is undefined because 'int' can hold only 31
> bits (excluding the sign bit), so some bits overflow"
> C Note: "The value '1024' is represented by 11 bits, allowing at most 20 bits
> for bitshift"
>
> Shortened:
> CXX Warning: "Undefined left shift: '1024' (11 bits) exceeds 'int' capacity
> (32 bits, including sign)"
> C Warning: "Undefined left shift: '1024' (11 bits) exceeds 'int' capacity (31
> bits, excluding sign)"
Clarification about the `Msg`/`ShortMsg` distinction:
I'm intentionally separating the short warning messages and the longer note
messages because `createBugReport()` enforces the convention that it will
always emit a warning and a note at the bug location.
According to the comments in the source code, the intention is that the note
contains all the relevant information, while the warning is a brief summary
that can be displayed in situations where the notes wouldn't fit the UI.
IIUC many checkers ignore this distinction and emit the same (often long and
cumbersome) message both as a note and as a warning (`createBugReport()` has a
variant which takes only one message parameter and passes it to both
locations), but I tried to follow it because I think it's ugly when the same
message is repeated twice and there is some sense in providing both a brief
summary and a full description that doesn't use potentially-ambiguous
abbreviations to save space.
Of course I could also accept a community decision that this "brief warning /
full note" distinction is deprecated and will be eliminated (because e.g.
front-end utilities are not prepared to handle it), but in that case I'd
strongly suggest a redesign where we eliminate the redundantly printed 'note'
message. (There is no reason to say the same thing twice! There is no reason to
say the same thing twice!)
On the other hand, in addition to this `Msg`/`ShortMsg` distinction, this part
of the code also adds the extra `LeftNote` (as a remnant from an earlier
internal version of this checker), and that's that's what I'd like to merge
into `Msg` (following NoQ's suggestion and keeping it distinct from the
`ShortMsg`).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156312/new/
https://reviews.llvm.org/D156312
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
