This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] bugprone-bool-pointer-implicit-conversion doesn't handle members
ClosedPublic

Authored by tetsuo-cpp on Jul 5 2020, 11:06 PM.

Details

Summary

Bugzilla: https://bugs.llvm.org/show_bug.cgi?id=45189
This patch is adding support for members to the bugprone-bool-pointer-implicit-conversion check. Apologies if I've done this in a weird/wrong way, I'm still getting my head around the matching DSL.

Diff Detail

Event Timeline

tetsuo-cpp created this revision.Jul 5 2020, 11:06 PM

Remove the unnecessary template.

Eugene.Zelenko set the repository for this revision to rG LLVM Github Monorepo.

Thanks for working on this.

There seems to be many places in this check where names are confusing, Var referring to a MemberExpr, DeclRef refering to a MemberExpr. I'd prefer if the names that you have added were renamed to something more in line with what they represent.

clang-tools-extra/clang-tidy/bugprone/BoolPointerImplicitConversionCheck.cpp
102

This should be an else if or the previous if should return. Saves redundant checking for MemberExpr when DeclRefExpr already matched.

clang-tools-extra/test/clang-tidy/checkers/bugprone-bool-pointer-implicit-conversion.cpp
110–128

nit: You don't need to recycle all the test cases, the logic for that is already checked above, just a few showing member expr being detected properly will suffice.

Don't have huge preference on this so feel free to ignore.

tetsuo-cpp updated this revision to Diff 276022.Jul 7 2020, 5:46 AM

Address comments: Rename variables, remove unnecessary tests and avoid unnecessary check.

tetsuo-cpp marked 2 inline comments as done.Jul 7 2020, 5:46 AM

Friendly ping!

Friendly ping. Could I please get this looked at?

LGTM, though I have a possible code simplification if you think it's an improvement.

clang-tools-extra/clang-tidy/bugprone/BoolPointerImplicitConversionCheck.cpp
78

Can you get away with:

if (const auto *E = Result.Nodes.getNodeAs<Expr>("expr")) {
  const Decl *D = isa<DeclRefExpr> ? cast<DeclRefExpr>(E)->getDecl() : cast<MemberExpr>(E)->getMemberDecl();
  const auto M = ignoringParenImpCasts(anyOf(declRefExpr(to(equalsNode(D))), memberExpr(hasDeclaration(equalsNode(D)))));
  checkImpl(Result, E, If, M, *this);
}

(Totally untested, but it hopefully demonstrates the point.)

Cleanup check conditional.

tetsuo-cpp marked an inline comment as done.Jul 23 2020, 5:46 AM

@aaron.ballman
Thanks! I took your suggestion and I think this looks cleaner.

Friendly ping @njames93.
Does this look ok to you? If so, could you please help me with the commit?

aaron.ballman accepted this revision.Aug 5 2020, 4:16 AM

Re-accepting the changes.

This revision is now accepted and ready to land.Aug 5 2020, 4:16 AM
aaron.ballman closed this revision.Aug 5 2020, 4:16 AM

Thank you for the patch, I've gone ahead and commit on your behalf in a44161692ae879068d4086a7e568a348800ba01d.