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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
