NoQ added inline comments.
================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:63
+
+ Gadget(Kind K) : K(K) {}
+
----------------
aaron.ballman wrote:
> Should this be an explicit constructor? (Same question applies below to
> derived classes as well.)
What's the value of making it `explicit` given that it's an abstract class that
can't be constructed directly anyway?
================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:113
+ : UnsafeGadget(Kind::Increment),
+ Op(Result.Nodes.getNodeAs<UnaryOperator>("op")) {}
+
----------------
aaron.ballman wrote:
> Should we make a `static constexpr const char *` for these strings so we can
> give it a name and ensure it's used consistently?
I think it makes perfect sense to have different bind-names in different
classes. They don't all correspond to the same role the bound expression plays.
================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:123
+ hasUnaryOperand(ignoringParenImpCasts(hasPointerType()))
+ ).bind("op"));
+ }
----------------
steakhal wrote:
> I thought this matcher was already bound as `"Increment"` by `x ##
> Gadget::matcher().bind(#x)` inside `GadgetFinderCallback::run()`
Yeah it's a bit redundant but it makes the class properly incapsulated, not
reliant on the external machine to use it in a certain way. A bit more
resistant to renaming as well.
And while //this time// these two bindings coincided, in the general case they
can be different. So I wanted to establish an idiom that works in all cases.
If we ever run into performance problems, I'm totally open to optimizing this.
================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:163
void run(const MatchFinder::MatchResult &Result) override {
+ // Figure out which matcher we've found, and call the appropriate
----------------
steakhal wrote:
> I wonder if we should assert that we only expect exactly one match (entry)
> inside `Result.Nodes`.
You mean like, exactly one if-statement succeeds? That'd require us to run the
entire list every time (at least in debug mode) but it's probably not too bad.
I'll look for a clean solution^^
================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:190-191
+#undef GADGET
+ // FIXME: Is there a better way to avoid hanging comma?
+ unless(stmt())
+ ))
----------------
steakhal wrote:
> I was thinking of constructing a `std::vector<DynTypedMatcher>` of the
> matchers of the disjunction because the extra comma is allowed inside the
> initializer expression.
> After that, you could probably use `constructVariadic(VO_AnyOf, ...)` as
> demonstrated by the `TEST(ConstructVariadic, MismatchedTypes_Regression)`
> unit-test. But TBH, I don't think it's more readable than this :D
One way or another, D138253 cleans up this FIXME.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D137348/new/
https://reviews.llvm.org/D137348
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
