[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

[Donát Nagy via Phabricator via cfe-commits]

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