steakhal marked an inline comment as done.
steakhal added a comment.
In D74806#1882300 <https://reviews.llvm.org/D74806#1882300>, @NoQ wrote:
> This is fantastic, i love it!
>
> > all tests are preserved and passing; only *the message changed at some
> > places*. In my opinion, these messages are holding the same information.
>
> You make it sound like an accident; i encourage you to have a closer look to
> make sure that there are no other effects than the ones observed on tests.
How should I look for differences/regressions without extending and validating
systematically the test cases we have for this checker? That seems to be a huge
work since `bstring.c` has 500+ and the `string.c` has 1600+ lines.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:36
+ const Expr *Expression;
+ unsigned ArgumentIndex;
+};
----------------
I'm not marking it `const` in this patch since the 496th line swaps two
instances of this class, so it cannot be done without refactoring that function.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:492-496
// Switch the values so that firstVal is before secondVal.
std::swap(firstLoc, secondLoc);
// Switch the Exprs as well, so that they still correspond.
std::swap(First, Second);
----------------
It is really interesting that it swaps the arguments in some sense.
I'm pretty sure that now if a diagnostic is emitted after this swap, the
argument indexes used to construct the diagnostic message would mismatch. I
will dig into this later, probably refactor the whole function to make the
intentions clear. What do you think? @NoQ
================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1578-1584
+ // FIXME(benics): Why do we choose the srcExpr if the access has no size?
+ // Note that the 3rd argument of the call would be the size parameter.
+ SizeArgExpr SrcExprAsSizeDummy = {srcExpr.Expression, srcExpr.ArgumentIndex};
+ state = CheckOverlap(
+ C, state,
+ (IsBounded ? SizeArgExpr{CE->getArg(2), 2} : SrcExprAsSizeDummy), Dst,
+ srcExpr);
----------------
This part is really interesting.
Using strong types highlighted this particular code. Since `SizeArgExpr` is
conceptually different than a `SourceArgExpr` this code did not compile without
additional effort.
Previously it passed the `size` argument if it were given, but if the function
were //unbounded// than it passed the `source` expression where `size` was
expected, which does not make any sense.
Since I plan to refactor the whole cstring related functions, not just the
`memset`, `memcpy` etc. I tried to leave it as it were, to preserve the current
//(broken)// semantics of the function.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D74806/new/
https://reviews.llvm.org/D74806
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
