[PATCH] D74806: [analyzer] NFCi: Refactor CStringChecker: use strongly typed internal API

[Balázs Benics via Phabricator via cfe-commits]

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