zaks.anna added a comment. Another partial review. Thanks! Anna.
================ Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:312 @@ +311,3 @@ + CheckerContext &C) const { + auto RetExpr = S->getRetValue(); + if (!RetExpr) ---------------- Please, explain what this method does and add a TODO comment if this needs got be improved in the future. ================ Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:332 @@ +331,3 @@ + ConditionTruthVal Nullness = + State->isNull(RetSVal.castAs<DefinedOrUnknownSVal>()); + bool IsNotNull = Nullness.isConstrainedFalse(); ---------------- You could cast and check "if (RetSVal.isUndef())" at the same time. ================ Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:376 @@ +375,3 @@ + +void NullabilityChecker::checkPreCall(const CallEvent &Call, + CheckerContext &C) const { ---------------- Please, add a high-level comment about what this method does. ================ Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:398 @@ +397,3 @@ + !Param->getType()->isReferenceType() && + !Param->getType()->isObjCObjectPointerType()) { + continue; ---------------- Should you use the existing helper function here as well? ================ Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:402 @@ +401,3 @@ + + ConditionTruthVal Nullness = + State->isNull(ArgSVal.castAs<DefinedOrUnknownSVal>()); ---------------- A lot of code here looks like a copy and paste from the "checkPreStmt(const ReturnStmt *S," above. Please, factor into a subroutine. ================ Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:453 @@ +452,3 @@ + // Marking memory regions of variables to be nullable would be a mistake. + // Marking otherwise is redundant. + if (!Region->getAs<SymbolicRegion>()) ---------------- I do not understand this comment. You are setting the nullability below.. ================ Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:463 @@ +462,3 @@ + if (State != OrigState) + C.addTransition(State); +} ---------------- Is calling a function/method with multiple arguments with set nullability tested? Looks like testArgumentTracking could be used for this but it is never called. ================ Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:475 @@ +474,3 @@ + QualType RetType = FuncType->getReturnType(); + if (!RetType->isPointerType() && !RetType->isObjCObjectPointerType()) + return; ---------------- Use a subroutine. ================ Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:496 @@ +495,3 @@ + CheckerContext &C) const { + auto Decl = M.getDecl(); + if (!Decl) ---------------- The next 11 lines could be factored out. They are the same as in the method above. ================ Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:513 @@ +512,3 @@ + auto Name = Interface ? Interface->getName() : ""; + // Frameworks related heuristics. + if (Name.startswith("NS")) { ---------------- Please, explain why we are ignoring these. EX: Users might know that objectForKey and objectForKeyedSubscript of NSDictionary always return a non-null value. It's dynamic invariant that the analyzer cannot infer. Why are we suppressing all methods on NSArray? ================ Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:524 @@ +523,3 @@ + + // Ignore the return value of string encoding methods. + if (Name.find("String") != StringRef::npos) { ---------------- Again, explain why we have this heuristic. ================ Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:540 @@ +539,3 @@ + + const ObjCMessageExpr *Message = M.getOriginExpr(); + Nullability SelfNullability = Nullability::Unspecified; ---------------- Maybe getting nullability of the receiver could be split into a separate function to simplify readability? ================ Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:542 @@ +541,3 @@ + Nullability SelfNullability = Nullability::Unspecified; + if (Message->getReceiverKind() == ObjCMessageExpr::SuperClass || + Message->getReceiverKind() == ObjCMessageExpr::SuperInstance) { ---------------- +comment // If the receiver is super, assume it's nonnull. ================ Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:545 @@ +544,3 @@ + SelfNullability = Nullability::Nonnull; + } else { + SVal Receiver = M.getReceiverSVal(); ---------------- +comment // Otherwise, look up the nullability info in the state. ================ Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:558 @@ +557,3 @@ + } + if (!Receiver.isUndef()) { + ConditionTruthVal Nullness = ---------------- + comment: // If we know that the receiver is constrained to not be null, assume it's nullability is Nonnull, regardless of what the type is. (Could this actually disagree with what's in the state?) ================ Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:566 @@ +565,3 @@ + + const NullabilityState *TrackedNullability = + State->get<NullabilityMap>(ReturnRegion); ---------------- TrackedNullability -> NullabilityOfReturn? Where is the nullability of the method declaration is bound / stored into the nullability of the return value? (I don't see it happening in the preCall..) I might be missing something. Is this for inlined cases? ================ Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:569 @@ +568,3 @@ + + if (TrackedNullability) { + Nullability RetValTracked = TrackedNullability->getValue(); ---------------- + comment ================ Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:629 @@ +628,3 @@ + if (!shouldTrackRegion(Region, C.getCurrentAnalysisDeclContext())) + return; + ---------------- Could all this code be factored into shouldTrackRegion helper function? auto RegionSVal = ExprSVal.getAs<loc::MemRegionVal>(); if (!RegionSVal) return; const MemRegion *Region = RegionSVal->getRegion(); if (!shouldTrackRegion(Region, C.getCurrentAnalysisDeclContext())) return; http://reviews.llvm.org/D11468 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits