[PATCH] D152435: [analyzer][CStringChecker] Adjust the invalidation operation on the super region of the destination buffer during string copy

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

steakhal added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:286-287
+                      SVal V,
+                      std::function<bool(RegionAndSymbolInvalidationTraits &,
+                                         const MemRegion *)>);
 
----------------
I'd highly suggest making this a template taking the functor that way.
Given that we modify these functions, we should make them comply with the lower camel case naming convention.
And their variables with upper camel case.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1054
+    SVal SizeV, QualType SizeTy) {
+  return InvalidateBufferAux(
+      C, S, BufE, BufV,
----------------
Shouldn't we assert that `SizeV` is some known value or at least smaller than (or equal to) the extent of the buffer?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1075-1077
+      [](RegionAndSymbolInvalidationTraits &, const MemRegion *R) {
+        return MemRegion::FieldRegionKind == R->getKind();
+      });
----------------
The lambdas inline in the call look a bit messy. I would prefer declaring it and passing it as an argument separately.
This applies to the rest of the lambdas as well.


================
Comment at: clang/test/Analysis/issue-55019.cpp:13-14
+
+void *malloc(size_t);
+void free(void *);
+
----------------
OikawaKirie wrote:
> > Ah, I see that it's for c function declarations. If that's the case, have you considered adding the malloc and free declarations to that header?
> 
> What about doing this in another patch and updating all test cases that use malloc and free? Maybe other libc APIs as well?
> The test case malloc-three-arg.c declares malloc and in a different signature and also includes this header. Simply doing so in this patch will lead to other conflicts.
Okay.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152435/new/

https://reviews.llvm.org/D152435