steakhal added inline comments.
================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:40
+
+ /// Determine if a kind is a safe kind. Slower than calling isSafe().
+ static bool isSafeKind(Kind K) {
----------------
We could have a `GADGET_RANGE(UnsafeGadgets, Increment, Decrement)`, which
could expand to `BEGIN_##x = Increment, END_##x = Decrement,` when declaring
the `Kind` enum, similarly how `SymExpr::Kind::Begin_BINARYSYMEXPRS` is defined.
That way this code could look like:
```lang=C++
return !(Kind::Begin_UnsafeGadgets <= K && K <= Kind::End_UnsafeGadgets);
```
================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:43-45
+#define UNSAFE_GADGET(x)
\
+ case Kind::x:
+#include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
----------------
I'd advocate for using the same macro variable name here as an XMACRO argument
as it's present in the definition as parameter.
Same for the rest of the XMACRO expansions.
================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:87
+ static bool classof(const Gadget *G) { return !isSafeKind(G->getKind()); }
+ bool isSafe() const override { return false; }
+};
----------------
================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:102
+ static bool classof(const Gadget *G) { return isSafeKind(G->getKind()); }
+ bool isSafe() const override { return true; }
+};
----------------
================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:123
+ hasUnaryOperand(ignoringParenImpCasts(hasPointerType()))
+ ).bind("op"));
+ }
----------------
I thought this matcher was already bound as `"Increment"` by `x ##
Gadget::matcher().bind(#x)` inside `GadgetFinderCallback::run()`
================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:150
+
+ const Stmt *getBaseStmt() const override { return Op; }
+};
----------------
I'd advocate for covariant return types for such cases.
================
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
----------------
I wonder if we should assert that we only expect exactly one match (entry)
inside `Result.Nodes`.
================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:190-191
+#undef GADGET
+ // FIXME: Is there a better way to avoid hanging comma?
+ unless(stmt())
+ ))
----------------
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
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
