donat.nagy marked 2 inline comments as done.
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));
----------------
donat.nagy wrote:
> donat.nagy wrote:
> > 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`).
> Among notes, my only planned change is that instead of
>
> > 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"
>
> I'll implement something like
>
> > Warning: "Left shift of '1024' overflows the capacity of 'int'"
> > CXX Note: "Left shift of '1024' (represented by 11 bits) is undefined
> > because 'int' can hold only 32 bits (including the sign bit), so some bits
> > overflow"
> > C Note: "Left shift of '1024' (represented by 11 bits) is undefined because
> > 'int' can hold only 31 bits (excluding the sign bit), so some bits overflow"
Correction: now I'm leaning towards just discarding the secondary note, because
the message examples listed in the previous comment are just the variants where
the right operand is unknown. In the more detailed message template "The shift
'{0} << {1}' is undefined {2}, so {3} bit{4} overflow{5}" there is no place to
insert the "represented by {} bits" message.
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
