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