A common pattern is that the code in the block does not write into the variable explicitly, but instead passes it to a helper function which performs the write.
Details
Diff Detail
- Repository
- rC Clang
Event Timeline
Sounds pretty aggressive. Are we sure that it's never a good idea to pass the pointer around? I don't fully understand what's wrong with using a wrapper to write into the error when it doesn't involve capturing it into a block.
The code looks reasonable.
lib/StaticAnalyzer/Checkers/ObjCAutoreleaseWriteChecker.cpp | ||
---|---|---|
118–132 | /dance * * | |
150–156 | Hmm, do we need an equalsBoundNode here? Why don't we simply inline the matcher? | |
169 | Would this be better with ignoringParenImpCasts? Or we could as well stop on parentheses as a weird suppression mechanism for the users. |
Sounds pretty aggressive
Actually, it's much less aggressive than the Clang warning, firing on *every* capture of autoreleasing variable in a block [unless you explicitly add an __autoreleasing lifetime specifier suppressing it]
Are we sure that it's never a good idea to pass the pointer around?
It's an almost guaranteed use-after-free if an autoreleasing pointer is written into inside the autoreleasing block (almost because maybe the pointer is never read from again).
It's OK to read it -- but then the user should pass *pointer, as that is sufficient. Passing just pointer almost guarantees a writing intent.
I don't fully understand what's wrong with using a wrapper to write into the error when it doesn't involve capturing it into a block.
It is captured in a block: the warning fires when we use a wrapper to write to the autoreleasing variable *already inside the block running in the autoreleasing pool*.
lib/StaticAnalyzer/Checkers/ObjCAutoreleaseWriteChecker.cpp | ||
---|---|---|
150–156 | Avoids mega-expressions, and we need the binding anyway. |
lib/StaticAnalyzer/Checkers/ObjCAutoreleaseWriteChecker.cpp | ||
---|---|---|
150–156 | I mean, declRefExpr(to(DoublePointerParamM)) sounds pretty short. And even if we need the binding, having equalsBoundNode() specifically is making things weirder because i'm starting to suspect that we're matching a pattern with backreferences while reading the code, even though in fact we don't. |
/dance