steakhal added a comment.
Great progress.
Now, moving to the docs.
Have you checked that the docs compile without errors and look as you would
expect? Please post how it looks.
Please make a note about this new checker in the clang's release docs, as we
usually announce new checkers and major changes there.
================
Comment at: clang/docs/analyzer/checkers.rst:58-64
+ int basic_examples(int a, int b) {
+ if (b < 0) {
+ return a >> b; // warn: right argument is negative
+ } else if (b >= 32) {
+ return a << b; // warn: right argument >= capacity
+ }
+ }
----------------
else after return
================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:272
+ if (const auto ConcreteRight = Right.getAs<nonloc::ConcreteInt>()) {
+ const std::int64_t RHS = ConcreteRight->getValue().getExtValue();
+ assert(RHS > MaximalAllowedShift);
----------------
`getExtValue()`, I believe, asserts that it "fits", thus the concrete int is at
most 64 bits wide. Does it work for `_BigInt`s? (or whatever we have like that
:D)
================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:299-303
+ if (Side == OperandSide::Left) {
+ NonNegOperands |= NonNegLeft;
+ } else {
+ NonNegOperands |= NonNegRight;
+ }
----------------
When I see something like this I always think of using a ternary.
Have you also considered?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:335-339
+ auto R =
+ std::make_unique<PathSensitiveBugReport>(BT, ShortMsg, Msg, ErrNode);
+ bugreporter::trackExpressionValue(ErrNode, Op->getLHS(), *R);
+ bugreporter::trackExpressionValue(ErrNode, Op->getRHS(), *R);
+ return R;
----------------
AHA! Now I got you. You also call this `R`.
Just use `R` at the callsite as well. Job done.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:355-357
+ if (!BTPtr)
+ BTPtr = std::make_unique<BugType>(this, "Bitwise shift",
+ "Suspicious operation");
----------------
How about eagerly inline initializing this field? And avoiding `mutable` as
well.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:367-369
+
+ const AnalyzerOptions &Opts = Mgr.getAnalyzerOptions();
+
----------------
================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:53
+class BitwiseShiftValidator {
+ // Primary mutable state:
+ CheckerContext &Ctx;
----------------
donat.nagy wrote:
> steakhal wrote:
> > Where does this comment corresponds to?
> The two data members (`Ctx` and `State`) that are directly below the comment.
>
> I tried to split the list of data members into three groups (primary mutable
> state, secondary mutable state, `const` members), but now I reconsider this
> I feel that these header comments are actually superfluous.
>
> I'll return to a variant of the earlier layout where I put
> `NonNegFlags`/`UpperBound` to the end and comment that they are only used for
> note tag creation; but don't emphasize the (immediately visible) `const`ness
> / non-`const`-ness of other members with comments.
One way to emphasize this is by choosing names like `FoldedState`, or anything
else that has something to do with "motion" or "change".
Given that if we have `ProgramStateRefs` in helper classes they usually remain
still. Think of BugReportVisitors for example. Consequently,it's important to
raise awareness when it's not like that. A different name would achieve this.
================
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:
> 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).
Okay, indeed one would need to define the `operator|(SignednessStatus,
SignednessStatus)` and `operato&|(SignednessStatus, SignednessStatus)` as well.
It probably doesn't worth it. Such a pity that we can't have nice things in C++.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:60-63
+ // If there is an upper bound, it is <= the size of a primitive type
+ // (expressed in bits), so this is a very safe sentinel value:
+ enum { NoUpperBound = 999 };
+ unsigned UpperBound = NoUpperBound;
----------------
donat.nagy wrote:
> steakhal wrote:
> > Have you considered using `std::optional`?
> > Given that the dimension of this variable is "bits", I think it would be
> > great to have that in its name.
> I considered using `std::optional`, but using this "old-school" solution
> ensures that `NoUpperBound` is comparable to and larger than the "real" upper
> bound values, which lets me avoid some ugly boolean logic. I'll update the
> comment and probably also the variable name.
What "ugly boolean logic" are you referring to?
I thought one can always use `value_or(999)` anyway. To me, that means that the
option approach is a refinement of yours. But depending on the situation, it
might be possible to avoid the extrema value 999 altogether.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:106-109
+ if (auto BRP = checkOvershift()) {
+ Ctx.emitReport(std::move(BRP));
+ return;
+ }
----------------
donat.nagy wrote:
> steakhal wrote:
> > I'd prefer `BR` or `Report`. Since we know already that we are in the
> > path-sensitive domain, `P` has less value there.
> > I'd apply this concept everywhere in the patch.
> The `P` stands for the "Ptr" in the name of the type alias `BugReportPtr`;
> but your comment clearly highlights that this variable name is confusing ;).
>
> I'll probably avoid the `auto` and use `BugReportPtr Bug = ...` to remind the
> reader that this is a pointer (that can be null) and if it's non-null, then
> we found a bug.
Oh I see. To me, it's fairly obvious now that bugreports are just
smart-pointers.
So I implicitly looked for some other reason. But just look at other checker,
and call it the same way how they do, for consistency. That's all I wanted.
There are two/three common options I believe: `B` or `BR`, and maybe `Report`.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:190-191
+ std::string Msg =
+ llvm::formatv("The result of {0} shift is undefined because the right "
+ "operand{1} is not smaller than {2}, the capacity of
'{3}'",
+ isLeftShift() ? "left" : "right", RightOpStr, LHSBitWidth,
----------------
donat.nagy wrote:
> steakhal wrote:
> >
> This is an important distinction, I use "not smaller than {2}" as a shorter
> synonym of "larger than or equal to {2}". I can choose some other wording if
> you feel that this is not clear enough.
No, I have not strong feelings. Feel free to use the one you like more.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:218
+ Side == OperandSide::Left ? "Left" :
"Right",
+ isLeftShift() ? "left" : "right")
+ .str();
----------------
donat.nagy wrote:
> 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.
Makes sense. It was so tempting though.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:315
+ default:
+ llvm_unreachable("this checker does not use other comparison operators");
+ }
----------------
donat.nagy wrote:
> steakhal wrote:
> >
> I'd say that the current wording is correct because the the checker "use"s
> the comparison operators (in its implementation) instead of "accept"ing them
> from outside; but I can replace the message with e.g. "this //function// does
> not accept other comparison operators" if you'd prefer that.
No strong feelings :)
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
