NoQ 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:
> > 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.
>
There's nothing wrong with long, multi-sentence diagnostic messages!
Unlike the compiler proper, we aren't typically used from the command line, so
we aren't trying to fit into 80 columns. So we start our warnings with a
capital letter and expect them to be, at the very least, complete sentences.
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
