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

[tra]

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.

[JonasToth]

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

[tra]

In D52179#1237194, @JonasToth wrote:

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]

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]

• Check that D is non-null

[tra]

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]

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]

LGTM!

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp

551–552 ↗

(On Diff #165792)

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

clang-tidy/readability/IdentifierNamingCheck.cpp

if (Mid.empty())
     Mid = "_";
   return (Style.Prefix + Mid + Style.Suffix).str();
 }
 
 static StyleKind findStyleKind(
     const NamedDecl *D,
     const std::vector<llvm::Optional<IdentifierNamingCheck::NamingStyle>>
         &NamingStyles) {
+  assert(D && D->getIdentifier() && !D->getName().empty() && !D->isImplicit() &&
+         "Decl must be an explicit identifier with a name.");
+
   if (isa<ObjCIvarDecl>(D) && NamingStyles[SK_ObjcIvar])
     return SK_ObjcIvar;
   
   if (isa<TypedefDecl>(D) && NamingStyles[SK_Typedef])
     return SK_Typedef;
 
   if (isa<TypeAliasDecl>(D) && NamingStyles[SK_TypeAlias])
     return SK_TypeAlias;
if (const auto *Decl = dyn_cast<VarDecl>(D)) {
     if (NamingStyles[SK_Variable])
       return SK_Variable;
 
     return SK_Invalid;
   }
 
   if (const auto *Decl = dyn_cast<CXXMethodDecl>(D)) {
     if (Decl->isMain() || !Decl->isUserProvided() ||
-        Decl->isUsualDeallocationFunction() ||
-        Decl->isCopyAssignmentOperator() || Decl->isMoveAssignmentOperator() ||
         Decl->size_overridden_methods() > 0)
       return SK_Invalid;
 
     if (Decl->isConstexpr() && NamingStyles[SK_ConstexprMethod])
       return SK_ConstexprMethod;
 
     if (Decl->isConstexpr() && NamingStyles[SK_ConstexprFunction])
       return SK_ConstexprFunction;