dcoughlin added a comment.
Nice work! This looks good, with some nits and testing comments inline. Have
you run this on real code? What kind of false positives do you see?
================
Comment at: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp:1853
+
+ void deriveParamLocation(CheckerContext &Ctx, SymbolRef sym);
+ void deriveAllocLocation(CheckerContext &Ctx, SymbolRef sym);
----------------
Let's dispense with tradition and add some doxygen comments describing what
these methods do.
================
Comment at: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp:2442
+ const DeclRegion *Region = dyn_cast<DeclRegion>(sym->getOriginRegion());
+ if (Region) {
----------------
LLVM style says to use `auto *` here rather than repeating the name of the type
twice.
================
Comment at: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp:2479
+ AllocStmt = PathDiagnosticLocation::getStmt(AllocNode);
+ // assert(AllocStmt && "Cannot find allocation statement");
+
----------------
If you don't want this assertion, you should remove it rather than comment it
out.
================
Comment at: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp:2524
+ deriveParamLocation(Ctx, sym);
+
+ createDescription(Ctx, GCEnabled, IncludeAllocationLine);
----------------
I'm worried that in the future it would be possible to reach createDescription
without the Location, UniqueingLocation, and UniqueingDecl being filled in. Can
you please add an assertion for this in createDescription().
================
Comment at: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp:2683
+ DefaultBool PerformCalleeSideParameterChecking;
+
----------------
malhar1995 wrote:
> This might be used in the future in case callee side parameter checking is
> added for Core Foundation and Objective-C objects.
Let's not add this if it is not used in the code.. You should either use this
to control the checking for your generalized case or remove it.
================
Comment at: test/Analysis/retain-release-inline.m:305
+void
callee_side_parameter_checking_leak(__attribute__((annotate("rc_ownership_consumed")))
isl_basic_map *bmap) { // expected-warning {{Potential leak}}
+}
----------------
Please include the entire diagnostic text in the expected-warning{{}}. This
will guarantee that future changes to the checker don't change it unexpectedly.
================
Comment at: test/Analysis/retain-release-inline.m:306
+void
callee_side_parameter_checking_leak(__attribute__((annotate("rc_ownership_consumed")))
isl_basic_map *bmap) { // expected-warning {{Potential leak}}
+}
+
----------------
I'd like to see some more tests. For example, you should get a warning if you
set the parameter to something else; also if you return it.
Do you have a test checking for that you don't warn when a consumed parameter
is correctly released?
Repository:
rL LLVM
https://reviews.llvm.org/D36441
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
