aaron.ballman added inline comments.
================ Comment at: lib/Sema/SemaChecking.cpp:2623 + QualType QT = Pointer->getType()->getPointeeType(); + if (!QT.isNull() && QT->isBooleanType()) + // Warn on bool* to bool conversion. ---------------- Quuxplusone wrote: > Quuxplusone wrote: > > efriedma wrote: > > > Have you considered skipping the isBooleanType() check? Passing any > > > pointer to a function which expects a boolean parameter seems > > > fundamentally suspicious. > > We already know that LLVM itself passes pointers to functions in many > > places, and they're not bugs. I mean, this isn't the first time someone's > > proposed that pointer->bool conversions are evil. :) > > > > I know this just came up on a mailing list somewhere recently (rsmith was > > involved in the rebuttal, as was I), but all I can find on Google is: > > https://groups.google.com/a/isocpp.org/d/msg/std-proposals/a3g9qXFVGCs/Wj40qj48H3wJ > > Anyway, the punch line I'm thinking of is something like > > ``` > > FooBar *FB = ...; > > LLVMAssert(FB, "expected fb to be set"); > > ``` > > but with whatever the real names are. Found a bunch of those when I wrote > > the diagnostic. And they're fairly solidly in the "not bugs" category. > > The thing about the bool*-only version is that bool pointers are rare in > > C++, so I'm not sure we're gaining much. But if we can't do something more > > general, there's still some benefit. > > I see your point about false positives for the more general version. I was > > sort of considering an attribute to allow implicit conversions to bool for > > a specific function (like an assertion), and then we could ban implicit > > conversions to bool for parameters to other functions. But that's probably > > overkill. > > Based on @sberg's feedback that this warning found no true positives, and > @Eugene.Zelenko's feedback that this warning already exists in clang-tidy > today, personally I favor the resolution of "leave this warning in clang-tidy > and abandon the attempt to move it into clang". > ... personally I favor the resolution of "leave this warning in clang-tidy > and abandon the attempt to move it into clang". This is my view as well. https://reviews.llvm.org/D45601 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
