Hi,
Please find the patch for PR16558. Please let me know your comments on the same.
Thanks!
Details
- Reviewers
jordan_rose
Diff Detail
Event Timeline
This looks quite reasonable. I have some general formatting notes, plus a request for more new tests -- in particular, one that tests the sort of wraparound that occurs in PR16558.
Thanks for working on this!
lib/StaticAnalyzer/Checkers/CStringChecker.cpp | ||
---|---|---|
673–680 | It's probably cheaper to do this section entirely in APSInt-land: llvm::APSInt fourInt = APSIntType(maxValInt).getValue(4); const llvm::APSInt &maxLengthInt = BVF.evalAPSInt(BO_Div, maxValInt, fourInt); NonLoc maxLength = svalBuilder.makeIntVal(maxLengthInt); | |
674 | We tend not to put bug numbers inline in the source, though we do include them in tests sometimes. (Source is timeless, tests are not.) | |
681–688 | It makes more sense to me to invert this, even though it's more nesting: if (Optional<NonLoc> strLn = strLength.getAs<NonLoc>()) { // ... } | |
686–687 | Style-wise we'd prefer to break within the argument list; if that doesn't work well we'd break after the equal sign, not the dot. |
Implemented review comments from jordan.
Added test case to test wraparound issue mentioned in PR16558.
Thanks, Karthik! I merged the PR16558 test into string.c so that we can keep invocations down, but otherwise this looks good. Committed with very minor changes in r188680.
We tend not to put bug numbers inline in the source, though we do include them in tests sometimes. (Source is timeless, tests are not.)