[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

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

donat.nagy marked 21 inline comments as done.
donat.nagy added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:29-30
+
+using namespace clang;
+using namespace ento;
+
----------------
steakhal wrote:
> This is used quite often and hinders the readability by indenting the args a lot. Making the spelling shorter, would somewhat mitigate this.
Good idea!


================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:57-59
+  // Secondary mutable state, used only for note tag creation:
+  enum { NonNegLeft = 1, NonNegRight = 2 };
+  unsigned NonNegFlags = 0;
----------------
donat.nagy wrote:
> steakhal wrote:
> > How about using the same enum type for these two? It would signify that these are used together.
> > Also, by only looking at the possible values, it's not apparent that these are bitflag values. A suffix might help the reader.
> Using the enum type is a good idea, however I'd probably put the `Flag` suffix into the name of the type:
> ```
> enum SignednessFlags : unsigned { NonNegLeft = 1, NonNegRight = 2 };
> SignednessFlags NonNegOperands = 0;
> ```
I rename the member `NonNegFlags` to `NonNegOperands` but its type remains a plain `unsigned` because otherwise using the or-assign operator like `NonNegOperands |= NonNegLeft` produces the error
> Assigning to 'enum SignednessFlags' from incompatible type 'unsigned int' (clang typecheck_convert_incompatible)
(note that both operands were from the same enum type).


================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:218
+                                       Side == OperandSide::Left ? "Left" : "Right",
+                                       isLeftShift() ? "left" : "right")
+                             .str();
----------------
steakhal wrote:
> The `isLeftShift() ? "left" : "right"` expression appears 4 times in total.
> I'd also recommend to not distinguish the capitalized "left" and "right" strings, and just post-process the messages to ensure that the first character is capitalized inside the `createBugReport()`.
> 
> Also note that you can reuse the same placeholder, so you would only need to pass the `side()` only once.
> ```lang=C++
> static void capitalizeBeginning(std::string &Str) {
>   if (Str.empty())
>     return;
>   Str[0] = llvm::toUpper(Str[0]);
> }
> 
> StringRef side() const {
>   return isLeftShift() ? "left" : "right";
> }
> ```
I introduced a method for `isLeftShift() ? "left" : "right"` (which is indeed repeated several times), but I didn't define `capitalizeBeginning` because that would've been used only once.

Your remark that [with the capitalization function] "you can reuse the same placeholder, so you would only need to pass the side() only once" seems to be incorrect, because `Side == OperandSide::Left ? "Left" : "Right"` (which operand are we talking about?) and `isLeftShift() ? "left" : "right"` (what kind of shift operator are we talking about?) are two independent values.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156312