Committed: r126187
On Feb 21, 2011, at 6:49 PM, Lenny Maiorani wrote:
>
> On Feb 11, 2011, at 9:55 PM, Ted Kremenek wrote:
>
>>
>> I think this looks very good, but I have a couple comments:
>>
>> + // If the check is for strnlen() then bind the return value to no more
>> than
>> + // the maxlen value.
>> + SVal maxlenVal;
>> + if (IsStrnlen) {
>> + const Expr *maxlenExpr = CE->getArg(1);
>> + maxlenVal = state->getSVal(maxlenExpr);
>> +
>> + NonLoc * strLengthNL = dyn_cast<NonLoc>(&strLength);
>> + NonLoc * maxlenValNL = dyn_cast<NonLoc>(&maxlenVal);
>> +
>> + QualType cmpTy = C.getSValBuilder().getContext().IntTy;
>> + const GRState *stateTrue, *stateFalse;
>> +
>> + // Check if the strLength is greater than or equal to the maxlen
>> + llvm::tie(stateTrue, stateFalse) =
>> + state->assume(cast<DefinedOrUnknownSVal>
>> + (C.getSValBuilder().evalBinOpNN(state, BO_GE,
>> + *strLengthNL,
>> *maxlenValNL,
>> + cmpTy)));
>> +
>> + // If the strLength is greater than or equal to the maxlen, set
>> strLength
>> + // to maxlen
>> + if (stateTrue && !stateFalse) {
>> + strLength = maxlenVal;
>> + }
>> + }
>> +
>>
>> Is there a reason 'maxlen' is declared outside if the 'if' block? It's not
>> used elsewhere. Why not just write:
>>
>> + if (IsStrnlen) {
>> + const Expr *maxlenExpr = CE->getArg(1);
>> + SVal maxlenVal = state->getSVal(maxlenExpr);
>> +
>>
>> I strongly prefer this style because it keeps the lifetime of variables
>> associated with the logic they are intended to be paired with, and no more.
>> By glancing at the code, I couldn't tell if maxlenVal was use elsewhere
>> unless I searched for it.
>>
>> Other minor nits:
>>
>> + NonLoc * strLengthNL = dyn_cast<NonLoc>(&strLength);
>> + NonLoc * maxlenValNL = dyn_cast<NonLoc>(&maxlenVal);
>>
>> please write:
>>
>> NonLoc *strLengthNL
>>
>> instead of:
>>
>> NonLoc * strLengthNL
>>
>> That matches the coding style of the majority of the codebase (which is
>> gradually moving in this direction).
>
> Attached is an updated version of the strnlen() checker. With the two changes
> you suggested, Ted. Please review.
>
> I think the SVal declaration was leftover due to some other changes I was
> making and somehow didn't get rid of it. As for where the '*' goes... noted.
> Thanks for the input.
>
> -Lenny
> <strnlen-checker.diff>
>
>
> --
> __o
> _`\<,_
> (*)/ (*)
> ~~~~~~~~~~~~~~~~~~~~
>
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
