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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
