zaks.anna added a comment. Add tests for checking element retrieval and tracking with ObjC subscript syntax for arrays and dictionaries.
Also see: http://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return ================ Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:334 @@ +333,3 @@ + +void ObjCGenericsChecker::checkPostObjCMessage(const ObjCMethodCall &M, + CheckerContext &C) const { ---------------- Add comment explaining that this callback is used to infer the types. Specifically, if a class method is called on a symbol, we use it to infer the type of the symbol. ================ Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:338 @@ +337,3 @@ + + ProgramStateRef State = C.getState(); + SymbolRef Sym = M.getReturnValue().getAsSymbol(); ---------------- Move State down, where it's used. ================ Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:346 @@ +345,3 @@ + if (MessageExpr->getReceiverKind() != ObjCMessageExpr::Class || + Sel.getAsString() != "class") + return; ---------------- Clarify what you are doing here. ================ Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:414 @@ +413,3 @@ + + // When the receiver type is id, or some super class of the tracked type (and + // kindof type), look up the method in the tracked type, not in the receiver ---------------- Split funding a tighter method definition into a subroutine. ================ Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:421 @@ +420,3 @@ + MessageExpr->getReceiverKind() == ObjCMessageExpr::Class) { + if (ASTCtxt.getObjCIdType() == ReceiverType || + ASTCtxt.getObjCClassType() == ReceiverType || ---------------- Should checking for id, class and kindof be a helper method? ================ Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:425 @@ +424,3 @@ + ASTCtxt.canAssignObjCInterfaces(ReceiverObjectPtrType, + *TrackedType))) { + const ObjCInterfaceDecl *InterfaceDecl = ---------------- What if ASTCtxt.canAssignObjCInterfaces is false here? Shouldn't we warn? Will we warn elsewhere? Let's make sure we have a test case. And add a comment. ================ Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:444 @@ +443,3 @@ + Optional<ArrayRef<QualType>> TypeArgs = + (*TrackedType)->getObjCSubstitutions(Method->getDeclContext()); + // This case might happen when there is an unspecialized override of a ---------------- Should we use the type on which this is defined and not the tracked type? ================ Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:453 @@ +452,3 @@ + // We can't do any type-checking on a type-dependent argument. + if (Arg->isTypeDependent()) + continue; ---------------- Why are we not checking other parameter types? Will those bugs get caught by casts? I guess yes.. ================ Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:458 @@ +457,3 @@ + + QualType OrigParamType = Param->getType(); + const auto *ParamTypedef = OrigParamType->getAs<TypedefType>(); ---------------- Why isObjCTypeParamDependent is not used here? ================ Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:485 @@ +484,3 @@ + if (ArgSym) { + const ObjCObjectPointerType *const *TrackedType = + State->get<TypeParamMap>(ArgSym); ---------------- Let's not shadow TrackedType. ================ Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:495 @@ +494,3 @@ + // accepted. + if (!ASTCtxt.canAssignObjCInterfaces(ParamObjectPtrType, + ArgObjectPtrType) && ---------------- This would be simplified if you pull out the negation; you essentially, list the cases where we do not warn on parameters: 1) arg can be assigned to (subtype of) param 2) covariant parameter types and param is a subtype of arg ================ Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:500 @@ +499,3 @@ + ParamObjectPtrType))) { + ExplodedNode *N = C.addTransition(); + reportBug(ArgObjectPtrType, ParamObjectPtrType, N, Sym, C); ---------------- This just returns the previous state. If you want to create a node, you need to tag it; see the tags on leak reports. ================ Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:513 @@ +512,3 @@ + ASTCtxt, *TypeArgs, ObjCSubstitutionContext::Result); + if (IsInstanceType) + ResultType = QualType(*TrackedType, 0); ---------------- We should not use TrackedType in the case lookup failed. Can the TrackedType be less specialized that the return type? Maybe rename as IsDeclaredAsInstanceType? ================ Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:532 @@ +531,3 @@ + + if (!ExprTypeAboveCast || !ResultPtrType) + return; ---------------- What are we doing here? I prefer not to have any AST pattern matching. ================ Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:539 @@ +538,3 @@ + !ASTCtxt.canAssignObjCInterfaces(ResultPtrType, ExprTypeAboveCast)) { + ExplodedNode *N = C.addTransition(); + reportBug(ResultPtrType, ExprTypeAboveCast, N, Sym, C); ---------------- This returns predecessor.. http://reviews.llvm.org/D11427 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
