Szelethus added a reviewer: gamesh411.
Szelethus added a comment.
Awesome, been a long time coming!!
Other than the minor observation of changing "of function" to "to", I'm
inclined to accept this patch. We definitely should describe what the value IS,
not just what is should be (aside from the cases concerning nullness, I think
they are fine as-is), but seems to be another can of worms deserving of its own
patch.
The reason why I'd solve the description of the argument separately is that
other checkers also suffer from this problem (`alpha.security.ArrayBoundV2`,
hello), and it opens conversations such as "What if we can't sensibly describe
that value?", and on occasions, we have felt that the go-to solution should be
just suppressing the warning. But then again, its an intentional false negative.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:915
Result += getArgDesc(ArgN);
- Result += DK == Violation ? " should not be NULL" : " is not NULL";
+ Result += " of function '";
+ Result += getFunctionName(Call);
----------------
Looking at the tests, the word 'function' may be redundant and unnecessary.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:932
Result += getArgDesc(ArgN);
- Result += DK == Violation ? " should be " : " is ";
-
- // Range kind as a string.
- Kind == OutOfRange ? Result += "out of" : Result += "within";
-
- // Get the range values as a string.
- Result += " the range ";
- if (Ranges.size() > 1)
- Result += "[";
- unsigned I = Ranges.size();
- for (const std::pair<RangeInt, RangeInt> &R : Ranges) {
- Result += "[";
- const llvm::APSInt &Min = BVF.getValue(R.first, T);
- const llvm::APSInt &Max = BVF.getValue(R.second, T);
- Min.toString(Result);
- Result += ", ";
- Max.toString(Result);
- Result += "]";
- if (--I > 0)
- Result += ", ";
+ Result += " of function '";
+ Result += getFunctionName(Call);
----------------
Here too.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:966
Result += getArgDesc(ArgN);
- Result += DK == Violation ? " should be " : " is ";
- Result += "equal to or greater than the value of ";
+ Result += " of function '";
+ Result += getFunctionName(Call);
----------------
Here too.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:980
}
return Result.c_str();
}
----------------
We are saying what the value should be, but neglect to say what it is instead.
Something like this would be good:
"The value of the 1st argument to _nonnull should be equal to or less then 10,
but is 11".
This is what I (and many of our users IIRC) miss in checker messages, like in
those where we are supposed to describe what the value of the index is (besides
it causing a buffer overflow).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143194/new/
https://reviews.llvm.org/D143194
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
