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

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SuspiciousReallocUsageCheck.cpp:89-90
+    return;
+  diag(Call->getBeginLoc(),
+       "memory block passed to 'realloc' may leak if 'realloc' fails");
+}
----------------
whisperity wrote:
> I have some reservations about this message, though. It only indirectly states the problem: first, the user needs to know what a memory leak is, and then needs to know how realloc could fail, and then make the mental connection about the overwriting of the pointer. (Also, you're passing a pointer... "memory block" is a more abstract term, requiring more knowledge.)
> 
> I think for the sake of clarity, we should be more explicit about this.
> 
> > taking the returned pointer from 'realloc' overwrites 'XXXPtr' with null
> > if allocation fails,
> > which may result in a leak of the original buffer
> 
> 1st line explains how the issue manifests (explicitly saying that you'll get a nullpointer in a bad place)
> 2nd line gives the condition, which is a bit more explicit
> 3rd line explains the real underlying issue
> 
> (Getting the null pointer is the "only" bad path here.)
Also:

```lang=cpp
diag(...) << PtrInputExpr.getSourceRange() << PtrResultExpr.getSourceRange();
```

So the connection that the recipient of the assignment and the 1st parameter is the same is more obvious.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133119