[clang-tidy] Replace redundant checks with an assert().
ClosedPublic

Authored by tra on Sep 17 2018, 11:20 AM.

Details

Summary

findStyleKind is only called if D is an explicit identifier with a name,
so the checks for operators will never return true. The explicit assert()
enforces this invariant.

Diff Detail

tra created this revision.Sep 17 2018, 11:20 AM
JonasToth set the repository for this revision to rCTE Clang Tools Extra.
JonasToth added a project: Restricted Project.
JonasToth added a subscriber: JonasToth.

Is the condition for this assertion checked beforehand or could this create runtime failures?

tra added a comment.Sep 17 2018, 12:57 PM

Is the condition for this assertion checked beforehand or could this create runtime failures?

It's checked by the (only) caller of the function on line 791:

  if (const auto *Decl = Result.Nodes.getNodeAs<NamedDecl>("decl")) {
    if (!Decl->getIdentifier() || Decl->getName().empty() || Decl->isImplicit())
      return;

...
    StyleKind SK = findStyleKind(Decl, NamingStyles);
    if (SK == SK_Invalid)
      return;
...
 }
aaron.ballman added inline comments.Sep 17 2018, 1:19 PM
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
388 ↗(On Diff #165792)

Can you also assert D is nonnull here? (isa<> used to take care of that, so it's a minor regression in checking.)

551–552 ↗(On Diff #165792)

Why are these being removed?

tra updated this revision to Diff 165841.Sep 17 2018, 3:31 PM
  • Check that D is non-null
tra marked an inline comment as done.Sep 17 2018, 4:00 PM
tra added inline comments.
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
551–552 ↗(On Diff #165792)

D51808 changes signature of isUsualDeallocationFunction(), which made me update the code here, which lead to @rsmith suggesting that this change is redundant and could use cleaning up, which led to this change.

Any Decl which would return true for isUsualDeallocationFunction() or for is*Operator(), will always return nullptr for getIdentifier() because D->getName().isIdentifier() would be false.

Speaking of which, the result of getIdentifier() is never used, so the check should probably be D->isIdentifier() both in this function and in the caller.

tra added inline comments.Sep 17 2018, 4:32 PM
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
388 ↗(On Diff #165841)

Speaking of which, the result of getIdentifier() is never used, so the check should probably be D->isIdentifier() both in this function and in the caller.

Scratch that. It's not worth the trouble, IMO. D->getDeclName().isIdentifier() is more verbose and does not buy us anything compared to D->getIdentifier() which expresses the intent well enough.

aaron.ballman accepted this revision.Sep 18 2018, 5:07 AM

LGTM!

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
551–552 ↗(On Diff #165792)

Ah, thank you for the explanation -- that makes sense.

This revision is now accepted and ready to land.Sep 18 2018, 5:07 AM
This revision was automatically updated to reflect the committed changes.