[PATCH] D133119: [clang-tidy] Add checker 'bugprone-suspicious-realloc-usage'.

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

I agree with @martong that `$ = realloc($, ...)` is indicative of something going wrong, even if the developer has saved the original value somewhere. Are we trying to catch this with Tidy specifically for this reason (instead of CSA's stdlib checker, or some taint-related mechanism?). However, @steakhal has some merit in saying that developers might prioritise the original over the copy, even if they made a copy. I think an easy solution from a documentation perspective is to have a test for this at the bottom of the test file. And document that we cannot (for one reason or the other) catch this supposed solution, **but** //if// you have made a copy already(!), then you might as well could have done `void* q = realloc(p, ...)` anyway! Having this documented leaves at least //some// paths open for us to fix false positives later, if they become a tremendous problem. (I have a gut feeling that they will not, that much.)



================
Comment at: clang-tools-extra/clang-tidy/bugprone/SuspiciousReallocUsageCheck.cpp:20-21
+namespace {
+class IsSamePtrExpr : public clang::StmtVisitor<IsSamePtrExpr, bool> /*,
+                         public clang::DeclVisitor<IsSamePtrExpr, bool>*/
+{
----------------
steakhal wrote:
> Commented code?
(Nit: `using namespace clang` -> The `clang::` should be redundant here.)


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SuspiciousReallocUsageCheck.cpp:87
+    return;
+  if (!IsSamePtrExpr().check(PtrInputExpr, PtrResultExpr))
+    return;
----------------
(We're creating a temporary here, right?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133119