NoQ marked an inline comment as done.
NoQ added inline comments.
================
Comment at: clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def:1
+//=- UnsafeBufferUsageGadgets.def - List of ways to use a buffer --*- C++
-*-=//
+//
----------------
NoQ wrote:
> aaron.ballman wrote:
> > Is this file named properly if it is also going to handle `SAFE_GADGET`?
> UnsafeBufferUsage is the name of the analysis. It's somewhat valuable to keep
> this file next to `UnsafeBufferUsage.h` so that it was obvious at a glance
> that these two files work together.
>
> I'm open to renaming the entire analysis though, a non-judgemental
> "BufferUsage analysis" totally works for me, WDYT?
>
> Or I can make a folder. But I don't expect more than 2 files in this folder.
I expect @malavikasamak to do some renaming in a follow-up patch in the near
future as she seeks to introduce more precise distinction between unsafe and
safe gadgets.
================
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) {
----------------
steakhal wrote:
> 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);
> ```
Nuked this entire facility. It's always possible to simply call `isSafe()`.
================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:87
+ static bool classof(const Gadget *G) { return !isSafeKind(G->getKind()); }
+ bool isSafe() const override { return false; }
+};
----------------
steakhal wrote:
>
I love that!
================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:107
+/// out of bounds.
+class IncrementGadget : public UnsafeGadget {
+ const UnaryOperator *Op;
----------------
NoQ wrote:
> xazax.hun wrote:
> > How deep will the `Gadget` Hierarchy be? Using inheritance only to classify
> > safe and unsafe gadgets feels like a very heavy weight solution to a
> > relatively simple problem.
> I'm expecting `UnsafeGadget` and `SafeGadget` to grow some common methods in
> the future. But I'm already second-guessing this decision
> (https://reviews.llvm.org/D137379?id=473092#inline-1340017) so I guess I'll
> ungrow these intermediate classes if things become too tedious.
To answer the original question, `SafeGadget` and `UnsafeGadget` are most
likely going to be the only intermediate class. So, it'll be 2 abstract classes
deep.
================
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
----------------
NoQ wrote:
> 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^^
Ok I added this huge `#ifdef NDEBUG` thing. I agree that it's valuable but I'm
also somewhat worried that it may diverge from the non-debug code.
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
