NoQ added inline comments.
================
Comment at: clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def:1
+//=- UnsafeBufferUsageGadgets.def - List of ways to use a buffer --*- C++
-*-=//
+//
----------------
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.
================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:17-21
+static auto hasPointerType() {
+ return anyOf(hasType(pointerType()),
+ hasType(autoType(hasDeducedType(
+ hasUnqualifiedDesugaredType(pointerType())))));
}
----------------
aaron.ballman wrote:
> Something to think about: ObjC pointers are pointer-like but not actual
> pointers; should those be covered as well (via `isAnyPointerType()`)?
Great question! Technically unsafe operations on ObjC pointers are not a
compile error, but they never actually make sense because you can't have
buffers of ObjC objects. Technically, you're not even supposed to know their
size or layout until runtime
(https://en.wikipedia.org/wiki/Objective-C#Non-fragile_instance_variables). So
this is an insanely exotic situation that we most likely won't be covering.
================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:37
+#include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
+#undef GADGETS
+ };
----------------
aaron.ballman wrote:
> NoQ wrote:
> > Whoops typo!
> We don't even need the `#undef` because the .def file already does that for
> us. Same observation applies below.
Whoops right.
================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:113
+ : UnsafeGadget(Kind::Increment),
+ Op(Result.Nodes.getNodeAs<UnaryOperator>("op")) {}
+
----------------
aaron.ballman wrote:
> NoQ wrote:
> > 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.
> Sorry, I was unclear. I meant a private data member of `IncrementGadget` so
> that the constructor and the `matcher()` functions use a named constant
> rather than a string literal.
Oh, fair enough!
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
