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

[Artem Belevich via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE342514: [clang-tidy] Replace redundant checks with an 
assert(). (authored by tra, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D52179?vs=165841&id=166042#toc

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52179

Files:
  clang-tidy/readability/IdentifierNamingCheck.cpp


Index: clang-tidy/readability/IdentifierNamingCheck.cpp
===
--- clang-tidy/readability/IdentifierNamingCheck.cpp
+++ clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -385,6 +385,9 @@
 const NamedDecl *D,
 const std::vector>
 &NamingStyles) {
+  assert(D && D->getIdentifier() && !D->getName().empty() && !D->isImplicit() 
&&
+ "Decl must be an explicit identifier with a name.");
+
   if (isa(D) && NamingStyles[SK_ObjcIvar])
 return SK_ObjcIvar;
   
@@ -548,8 +551,6 @@
 
   if (const auto *Decl = dyn_cast(D)) {
 if (Decl->isMain() || !Decl->isUserProvided() ||
-Decl->isUsualDeallocationFunction() ||
-Decl->isCopyAssignmentOperator() || Decl->isMoveAssignmentOperator() ||
 Decl->size_overridden_methods() > 0)
   return SK_Invalid;
 


Index: clang-tidy/readability/IdentifierNamingCheck.cpp
===
--- clang-tidy/readability/IdentifierNamingCheck.cpp
+++ clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -385,6 +385,9 @@
 const NamedDecl *D,
 const std::vector>
 &NamingStyles) {
+  assert(D && D->getIdentifier() && !D->getName().empty() && !D->isImplicit() &&
+ "Decl must be an explicit identifier with a name.");
+
   if (isa(D) && NamingStyles[SK_ObjcIvar])
 return SK_ObjcIvar;
   
@@ -548,8 +551,6 @@
 
   if (const auto *Decl = dyn_cast(D)) {
 if (Decl->isMain() || !Decl->isUserProvided() ||
-Decl->isUsualDeallocationFunction() ||
-Decl->isCopyAssignmentOperator() || Decl->isMoveAssignmentOperator() ||
 Decl->size_overridden_methods() > 0)
   return SK_Invalid;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM!




Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:551-552
 if (Decl->isMain() || !Decl->isUserProvided() ||
-Decl->isUsualDeallocationFunction() ||
-Decl->isCopyAssignmentOperator() || Decl->isMoveAssignmentOperator() ||
 Decl->size_overridden_methods() > 0)

tra wrote:
> aaron.ballman wrote:
> > Why are these being removed?
> 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.
> 
Ah, thank you for the explanation -- that makes sense.


https://reviews.llvm.org/D52179



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:388
 &NamingStyles) {
+  assert(D && D->getIdentifier() && !D->getName().empty() && !D->isImplicit() 
&&
+ "Decl must be an explicit identifier with a name.");

> 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.



https://reviews.llvm.org/D52179



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Belevich via Phabricator via cfe-commits]

tra marked an inline comment as done.
tra added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:551-552
 if (Decl->isMain() || !Decl->isUserProvided() ||
-Decl->isUsualDeallocationFunction() ||
-Decl->isCopyAssignmentOperator() || Decl->isMoveAssignmentOperator() ||
 Decl->size_overridden_methods() > 0)

aaron.ballman wrote:
> Why are these being removed?
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.



https://reviews.llvm.org/D52179



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Belevich via Phabricator via cfe-commits]

tra updated this revision to Diff 165841.
tra added a comment.

- Check that D is non-null


https://reviews.llvm.org/D52179

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


Index: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -385,6 +385,9 @@
 const NamedDecl *D,
 const std::vector>
 &NamingStyles) {
+  assert(D && D->getIdentifier() && !D->getName().empty() && !D->isImplicit() 
&&
+ "Decl must be an explicit identifier with a name.");
+
   if (isa(D) && NamingStyles[SK_ObjcIvar])
 return SK_ObjcIvar;
   
@@ -548,8 +551,6 @@
 
   if (const auto *Decl = dyn_cast(D)) {
 if (Decl->isMain() || !Decl->isUserProvided() ||
-Decl->isUsualDeallocationFunction() ||
-Decl->isCopyAssignmentOperator() || Decl->isMoveAssignmentOperator() ||
 Decl->size_overridden_methods() > 0)
   return SK_Invalid;
 


Index: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -385,6 +385,9 @@
 const NamedDecl *D,
 const std::vector>
 &NamingStyles) {
+  assert(D && D->getIdentifier() && !D->getName().empty() && !D->isImplicit() &&
+ "Decl must be an explicit identifier with a name.");
+
   if (isa(D) && NamingStyles[SK_ObjcIvar])
 return SK_ObjcIvar;
   
@@ -548,8 +551,6 @@
 
   if (const auto *Decl = dyn_cast(D)) {
 if (Decl->isMain() || !Decl->isUserProvided() ||
-Decl->isUsualDeallocationFunction() ||
-Decl->isCopyAssignmentOperator() || Decl->isMoveAssignmentOperator() ||
 Decl->size_overridden_methods() > 0)
   return SK_Invalid;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:388
 &NamingStyles) {
+  assert(D->getIdentifier() && !D->getName().empty() && !D->isImplicit() &&
+ "Decl must be an explicit identifier with a name.");

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



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:551-552
 if (Decl->isMain() || !Decl->isUserProvided() ||
-Decl->isUsualDeallocationFunction() ||
-Decl->isCopyAssignmentOperator() || Decl->isMoveAssignmentOperator() ||
 Decl->size_overridden_methods() > 0)

Why are these being removed?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52179



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

In https://reviews.llvm.org/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("decl")) {
  if (!Decl->getIdentifier() || Decl->getName().empty() || 
Decl->isImplicit())
return;
  
  ...
  StyleKind SK = findStyleKind(Decl, NamingStyles);
  if (SK == SK_Invalid)
return;
  ...
   }


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52179



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

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


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52179



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Belevich via Phabricator via cfe-commits]

tra created this revision.
tra added reviewers: alexfh, rsmith.
Herald added subscribers: bixia, xazax.hun, jlebar, sanjoy.

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.


https://reviews.llvm.org/D52179

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


Index: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -385,6 +385,9 @@
 const NamedDecl *D,
 const std::vector>
 &NamingStyles) {
+  assert(D->getIdentifier() && !D->getName().empty() && !D->isImplicit() &&
+ "Decl must be an explicit identifier with a name.");
+
   if (isa(D) && NamingStyles[SK_ObjcIvar])
 return SK_ObjcIvar;
   
@@ -548,8 +551,6 @@
 
   if (const auto *Decl = dyn_cast(D)) {
 if (Decl->isMain() || !Decl->isUserProvided() ||
-Decl->isUsualDeallocationFunction() ||
-Decl->isCopyAssignmentOperator() || Decl->isMoveAssignmentOperator() ||
 Decl->size_overridden_methods() > 0)
   return SK_Invalid;
 


Index: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -385,6 +385,9 @@
 const NamedDecl *D,
 const std::vector>
 &NamingStyles) {
+  assert(D->getIdentifier() && !D->getName().empty() && !D->isImplicit() &&
+ "Decl must be an explicit identifier with a name.");
+
   if (isa(D) && NamingStyles[SK_ObjcIvar])
 return SK_ObjcIvar;
   
@@ -548,8 +551,6 @@
 
   if (const auto *Decl = dyn_cast(D)) {
 if (Decl->isMain() || !Decl->isUserProvided() ||
-Decl->isUsualDeallocationFunction() ||
-Decl->isCopyAssignmentOperator() || Decl->isMoveAssignmentOperator() ||
 Decl->size_overridden_methods() > 0)
   return SK_Invalid;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits