[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 added a comment.

I like this, especially, that the functionality of UBOR can be merged and extended in a much cleaner way (architecturally).



================
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:
> 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)"


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156312/new/

https://reviews.llvm.org/D156312