[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

hintonda updated this revision to Diff 193376.
hintonda added a comment.

- Renamed and updated docs.
- Fix formatting.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802

Files:
  clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
  clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.cpp
  clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/llvm-prefer-isa-or-dyn-cast-in-conditionals.rst
  
clang-tools-extra/test/clang-tidy/llvm-prefer-isa-or-dyn-cast-in-conditionals.cpp

Index: clang-tools-extra/test/clang-tidy/llvm-prefer-isa-or-dyn-cast-in-conditionals.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/llvm-prefer-isa-or-dyn-cast-in-conditionals.cpp
@@ -0,0 +1,118 @@
+// RUN: %check_clang_tidy %s llvm-prefer-isa-or-dyn-cast-in-conditionals %t
+
+struct X;
+struct Y;
+struct Z {
+  int foo();
+  X *bar();
+};
+
+template 
+bool isa(Y *);
+template 
+X *cast(Y *);
+template 
+X *dyn_cast(Y *);
+template 
+X *dyn_cast_or_null(Y *);
+
+bool foo(Y *y, Z *z) {
+  if (auto x = cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:16: warning: cast<> in conditional will assert rather than return a null pointer [llvm-prefer-isa-or-dyn-cast-in-conditionals]
+  // CHECK-FIXES: if (auto x = dyn_cast(y))
+
+  while (auto x = cast(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:19: warning: cast<> in conditional
+  // CHECK-FIXES: while (auto x = dyn_cast(y))
+
+  if (cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: cast<> in conditional
+  // CHECK-FIXES: if (isa(y))
+
+  while (cast(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:10: warning: cast<> in conditional
+  // CHECK-FIXES: while (isa(y))
+
+  do {
+break;
+  } while (cast(y));
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: cast<> in conditional
+  // CHECK-FIXES: while (isa(y));
+
+  if (dyn_cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: return value from dyn_cast<> not used [llvm-prefer-isa-or-dyn-cast-in-conditionals]
+  // CHECK-FIXES: if (isa(y))
+
+  while (dyn_cast(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:10: warning: return value from dyn_cast<> not used
+  // CHECK-FIXES: while (isa(y))
+
+  do {
+break;
+  } while (dyn_cast(y));
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: return value from dyn_cast<> not used
+  // CHECK-FIXES: while (isa(y));
+
+  if (z->bar() && isa(z->bar()))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  method '{{.*}}' is called twice and could be expensive [llvm-prefer-isa-or-dyn-cast-in-conditionals]
+  // CHECK-FIXES: if (dyn_cast_or_null(z->bar()))
+
+  if (z->bar() && cast(z->bar()))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: method {{'.*'}} is called twice
+  // CHECK-FIXES: if (dyn_cast_or_null(z->bar()))
+
+  if (z->bar() && dyn_cast(z->bar()))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: method {{'.*'}} is called twice
+  // CHECK-FIXES: if (dyn_cast_or_null(z->bar()))
+
+  if (z->bar() && dyn_cast_or_null(z->bar()))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: method {{'.*'}} is called twice
+  // CHECK-FIXES: if (dyn_cast_or_null(z->bar()))
+
+  bool b = z->bar() && cast(z->bar());
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: method {{'.*'}} is called twice
+  // CHECK-FIXES: bool b = dyn_cast_or_null(z->bar());
+
+  // These don't trigger a warning.
+  if (auto x = cast(y)->foo())
+return true;
+  while (auto x = cast(y)->foo())
+break;
+  if (cast(y)->foo())
+return true;
+  while (cast(y)->foo())
+break;
+  if (y && isa(y))
+return true;
+  if (y && cast(z->bar()))
+return true;
+  if (z && cast(y)->foo())
+return true;
+  bool b2 = y && cast(z);
+
+#define CAST(T, Obj) cast(Obj)
+#define AUTO_VAR_CAST(X, Y, Z) auto X = cast(Z)
+#define ISA(T, Obj) isa(Obj)
+#define ISA_OR_NULL(T, Obj) Obj &(Obj)
+
+  if (auto x = CAST(X, y))
+return true;
+  if (AUTO_VAR_CAST(x, X, z))
+return true;
+  if (z->bar() && ISA(Y, z->bar()))
+return true;
+  if (ISA_OR_NULL(Y, z->bar()))
+return true;
+
+  return false;
+}
Index: clang-tools-extra/docs/clang-tidy/checks/llvm-prefer-isa-or-dyn-cast-in-conditionals.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/llvm-prefer-isa-or-dyn-cast-in-conditionals.rst
@@ -0,0 +1,33 @@
+.. title:: clang-tidy - llvm-prefer-isa-or-dyn-cast-in-conditionals
+
+llvm-prefer-isa-or-dyn-cast-in-conditionals
+===
+
+  Looks at conditionals and finds cases of ``cast<>``, which will

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:18
+
+AST_MATCHER(Expr, isMacroID) { return Node.getExprLoc().isMacroID(); }
+} // namespace ast_matchers

@aaron.ballman:  This matcher seems genuinely useful.  What do you think about 
moving it to ASTMatchers.h? 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

hintonda updated this revision to Diff 193176.
hintonda added a comment.

- Defer HeaderFileExtionsUtils.h bugfix to separate patch.
- Simplify code per comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802

Files:
  clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp
  clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.h
  clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
  clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp

Index: clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
@@ -0,0 +1,118 @@
+// RUN: %check_clang_tidy %s llvm-avoid-cast-in-conditional %t
+
+struct X;
+struct Y;
+struct Z {
+  int foo();
+  X *bar();
+};
+
+template 
+bool isa(Y *);
+template 
+X *cast(Y *);
+template 
+X *dyn_cast(Y *);
+template 
+X *dyn_cast_or_null(Y *);
+
+bool foo(Y *y, Z *z) {
+  if (auto x = cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:16: warning: cast<> in conditional will assert rather than return a null pointer [llvm-avoid-cast-in-conditional]
+  // CHECK-FIXES: if (auto x = dyn_cast(y))
+
+  while (auto x = cast(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:19: warning: cast<> in conditional
+  // CHECK-FIXES: while (auto x = dyn_cast(y))
+
+  if (cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: cast<> in conditional
+  // CHECK-FIXES: if (isa(y))
+
+  while (cast(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:10: warning: cast<> in conditional
+  // CHECK-FIXES: while (isa(y))
+
+  do {
+break;
+  } while (cast(y));
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: cast<> in conditional
+  // CHECK-FIXES: while (isa(y));
+
+  if (dyn_cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: return value from dyn_cast<> not used [llvm-avoid-cast-in-conditional]
+  // CHECK-FIXES: if (isa(y))
+
+  while (dyn_cast(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:10: warning: return value from dyn_cast<> not used
+  // CHECK-FIXES: while (isa(y))
+
+  do {
+break;
+  } while (dyn_cast(y));
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: return value from dyn_cast<> not used
+  // CHECK-FIXES: while (isa(y));
+
+  if (z->bar() && isa(z->bar()))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  method '{{.*}}' is called twice and could be expensive [llvm-avoid-cast-in-conditional]
+  // CHECK-FIXES: if (dyn_cast_or_null(z->bar()))
+
+  if (z->bar() && cast(z->bar()))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: method {{'.*'}} is called twice
+  // CHECK-FIXES: if (dyn_cast_or_null(z->bar()))
+
+  if (z->bar() && dyn_cast(z->bar()))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: method {{'.*'}} is called twice
+  // CHECK-FIXES: if (dyn_cast_or_null(z->bar()))
+
+  if (z->bar() && dyn_cast_or_null(z->bar()))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: method {{'.*'}} is called twice
+  // CHECK-FIXES: if (dyn_cast_or_null(z->bar()))
+
+  bool b = z->bar() && cast(z->bar());
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: method {{'.*'}} is called twice
+  // CHECK-FIXES: bool b = dyn_cast_or_null(z->bar());
+
+  // These don't trigger a warning.
+  if (auto x = cast(y)->foo())
+return true;
+  while (auto x = cast(y)->foo())
+break;
+  if (cast(y)->foo())
+return true;
+  while (cast(y)->foo())
+break;
+  if (y && isa(y))
+return true;
+  if (y && cast(z->bar()))
+return true;
+  if (z && cast(y)->foo())
+return true;
+  bool b2 = y && cast(z);
+
+#define CAST(T, Obj) cast(Obj)
+#define AUTO_VAR_CAST(X, Y, Z) auto X = cast(Z)
+#define ISA(T, Obj) isa(Obj)
+#define ISA_OR_NULL(T, Obj) Obj &(Obj)
+
+  if (auto x = CAST(X, y))
+return true;
+  if (AUTO_VAR_CAST(x, X, z))
+return true;
+  if (z->bar() && ISA(Y, z->bar()))
+return true;
+  if (ISA_OR_NULL(Y, z->bar()))
+return true;
+
+  return false;
+}
Index: clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
@@ -0,0 +1,28 @@
+.. title:: clang-tidy - llvm-avoid-cast-in-conditional
+
+llvm-avoid-cast-in-conditional
+==
+
+  Finds uses of ``cast<>`` in conditionals of ``if``, ``while`` or
+  ``do`` statements, which will assert rather than return a null
+  pointer. It also finds uses of ``dyn_cast<>`` in conditionals where
+  the 

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

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



Comment at: 
clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:145
+
+diag(MatchedDecl->getBeginLoc(), "use dyn_cast_or_null")
+<< FixItHint::CreateReplacement(SourceRange(MatchedDecl->getBeginLoc(),

aaron.ballman wrote:
> hintonda wrote:
> > aaron.ballman wrote:
> > > This diagnostic doesn't tell the user what they've done wrong with the 
> > > code or why this is a better choice.
> > Yes, but I'm not yet sure what it should say.  Was sorta hoping for a 
> > suggestion.  
> Do you have any evidence that this situation happens in practice? I kind of 
> feel like this entire branch could be eliminated from this patch unless it 
> actually catches problems that happen.
Yes, here are a few from clang/lib -- let me know if you think it's worth it or 
not to keep this:

- DiagnosticName: llvm-avoid-cast-in-conditional
  FileOffset: 305293
  FilePath: 
/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/SemaTemplate.cpp
  Message: method 'getAsTemplateDecl' is called twice and could be expensive
  Replacements:
  - FilePath: 
/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/SemaTemplate.cpp
Length: 93
Offset: 305293
ReplacementText: 
dyn_cast_or_null(Name.getAsTemplateDecl())
- DiagnosticName: llvm-avoid-cast-in-conditional
  FileOffset: 153442
  FilePath: 
/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/AST/ASTContext.cpp
  Message: method 'getAsTemplateDecl' is called twice and could be expensive
  Replacements:
  - FilePath: 
/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/AST/ASTContext.cpp
Length: 92
Offset: 153442
ReplacementText: 
dyn_cast_or_null(Template.getAsTemplateDecl())
- DiagnosticName: llvm-avoid-cast-in-conditional
  FileOffset: 97556
  FilePath: 
/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/AST/Expr.cpp
  Message: method 'getMethodDecl' is called twice and could be expensive
  Replacements:
  - FilePath: 
/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/AST/Expr.cpp
Length: 68
Offset: 97556
ReplacementText: dyn_cast_or_null(MCE->getMethodDecl())
- DiagnosticName: llvm-avoid-cast-in-conditional
  FileOffset: 301950
  FilePath: 
/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/SemaInit.cpp
  Message: method 'get' is called twice and could be expensive
  Replacements:
  - FilePath: 
/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/SemaInit.cpp
Length: 49
Offset: 301950
ReplacementText: dyn_cast_or_null(CurInit.get())
- DiagnosticName: llvm-avoid-cast-in-conditional
  FileOffset: 14335
  FilePath: 
/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/AnalysisBasedWarnings.cpp
  Message: method 'operator bool' is called twice and could be expensive
  Replacements:
  - FilePath: 
/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/AnalysisBasedWarnings.cpp
Length: 57
Offset: 14335
ReplacementText: dyn_cast_or_null(B->getTerminator())
- DiagnosticName: llvm-avoid-cast-in-conditional
  FileOffset: 15997
  FilePath: 
/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/AnalysisBasedWarnings.cpp
  Message: method 'operator bool' is called twice and could be expensive
  Replacements:
  - FilePath: 
/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/AnalysisBasedWarnings.cpp
Length: 55
Offset: 15997
ReplacementText: dyn_cast_or_null(B.getTerminator())
- DiagnosticName: llvm-avoid-cast-in-conditional
  FileOffset: 9492
  FilePath: 
/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
  Message: method 'sexpr' is called twice and could be expensive
  Replacements:
  - FilePath: 
/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
Length: 39
Offset: 9492
ReplacementText: dyn_cast_or_null(sexpr())
- DiagnosticName: llvm-avoid-cast-in-conditional
  FileOffset: 9572
  FilePath: 
/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
  Message: method 'sexpr' is called twice and could be expensive
  Replacements:
  - FilePath: 
/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
Length: 38
Offset: 9572
ReplacementText: dyn_cast_or_null(sexpr())
- DiagnosticName: llvm-avoid-cast-in-conditional
  FileOffset: 9492
  FilePath: 
/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
  Message: method 'sexpr' is called twice and could be expensive
  Replacements:
  - FilePath: 

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In D59802#1450185 , @xbolva00 wrote:

> Do we have any checker to recommend llvm functions over std ?
>
> e.g. llvm::sort, llvm::all_of, ...


No. Actually LLVM coding conventions support is very limited. See 
https://clang.llvm.org/extra/clang-tidy/checks/list.html.

I think check for auto usage is useful too.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

Do we have any checker to recommend llvm functions over std ?

e.g. llvm::sort, llvm::all_of, ...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

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



Comment at: 
clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:125-126
+
+diag(MatchedDecl->getBeginLoc(), "return value from dyn_cast<> not used")
+<< FixItHint::CreateReplacement(SourceRange(StartLoc, EndLoc), "isa");
+  } else if (const auto *MatchedDecl =

aaron.ballman wrote:
> There are zero test cases that seem to trigger this diagnostic text.
Sorry for any confusion, but there are actually 3 tests for this below.  
That'll be made clearer once I address your comments below, and spell out the 
entire diagnostic message on the first instance.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h:21
 
-typedef llvm::SmallSet HeaderFileExtensionsSet;
+using HeaderFileExtensionsSet = ::llvm::SmallSet;
 

aaron.ballman wrote:
> hintonda wrote:
> > aaron.ballman wrote:
> > > Can you add back the `::llvm::` qualifier on the `StringRef` type?
> > I could do that, but do you mean just this case of StringRef, of all four?  
> > And what about SourceLocation and SourceManager?
> If any changes are needed in this file at all, I'd prefer to keep them 
> minimal and self-consistent, so only this instance. However, I wouldn't be 
> opposed to a consistency cleanup in this file as a separate patch with NFC.
Since fixing the bug in this file is orthogonal to this patch, I'll address it 
in it's own patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h:21
 
-typedef llvm::SmallSet HeaderFileExtensionsSet;
+using HeaderFileExtensionsSet = ::llvm::SmallSet;
 

hintonda wrote:
> aaron.ballman wrote:
> > Can you add back the `::llvm::` qualifier on the `StringRef` type?
> I could do that, but do you mean just this case of StringRef, of all four?  
> And what about SourceLocation and SourceManager?
If any changes are needed in this file at all, I'd prefer to keep them minimal 
and self-consistent, so only this instance. However, I wouldn't be opposed to a 
consistency cleanup in this file as a separate patch with NFC.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h:21
 
-typedef llvm::SmallSet HeaderFileExtensionsSet;
+using HeaderFileExtensionsSet = ::llvm::SmallSet;
 

aaron.ballman wrote:
> Can you add back the `::llvm::` qualifier on the `StringRef` type?
I could do that, but do you mean just this case of StringRef, of all four?  And 
what about SourceLocation and SourceManager?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:38
+  whileStmt(anyOf(
+  has(declStmt(containsDeclaration(
+  0, varDecl(

aaron.ballman wrote:
> This seems like a fair amount of code duplication -- can this be cleaned up 
> using some local variables for the inner matchers?
I'm still hoping that some of this duplication can be reduced, btw.



Comment at: 
clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:145
+
+diag(MatchedDecl->getBeginLoc(), "use dyn_cast_or_null")
+<< FixItHint::CreateReplacement(SourceRange(MatchedDecl->getBeginLoc(),

hintonda wrote:
> aaron.ballman wrote:
> > This diagnostic doesn't tell the user what they've done wrong with the code 
> > or why this is a better choice.
> Yes, but I'm not yet sure what it should say.  Was sorta hoping for a 
> suggestion.  
Do you have any evidence that this situation happens in practice? I kind of 
feel like this entire branch could be eliminated from this patch unless it 
actually catches problems that happen.



Comment at: 
clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:103-104
+
+//if (StartLoc.isMacroID())
+//  return;
+

Spurious code that can be removed?



Comment at: 
clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:118
+ "cast<> in conditional will assert rather than return a null pointer")
+<< FixItHint::CreateReplacement(SourceRange(StartLoc, EndLoc), "isa");
+  } else if (const auto *MatchedDecl =

Aside from the fix-it, this code is identical to the block above. Can be 
combined.



Comment at: 
clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:125-126
+
+diag(MatchedDecl->getBeginLoc(), "return value from dyn_cast<> not used")
+<< FixItHint::CreateReplacement(SourceRange(StartLoc, EndLoc), "isa");
+  } else if (const auto *MatchedDecl =

There are zero test cases that seem to trigger this diagnostic text.



Comment at: clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h:21
 
-typedef llvm::SmallSet HeaderFileExtensionsSet;
+using HeaderFileExtensionsSet = ::llvm::SmallSet;
 

Can you add back the `::llvm::` qualifier on the `StringRef` type?



Comment at: 
clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp:22
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:16: warning: {{cast<> in conditional 
.*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (auto x = dyn_cast(y))

Please spell out the full diagnostic the first time it occur in the test case - 
it's fine to abbreviate subsequent diagnostics, but we like to see at least one 
exact match per diagnostic.



Comment at: 
clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp:48
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{.*dyn_cast<> not used 
.*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (isa(y))

Same here.



Comment at: 
clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp:64
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{method .* is called twice 
.*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (dyn_cast_or_null(z->bar()))

and here


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

hintonda added a comment.

In D59802#1448574 , @hintonda wrote:

> - Added  matcher, and reordered tests.


Commandline ate my ticks... ;-(

Should be added `isMacroID` matcher.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

hintonda updated this revision to Diff 192939.
hintonda added a comment.

- Added  matcher, and reordered tests.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802

Files:
  clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp
  clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.h
  clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
  clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
  clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp

Index: clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
@@ -0,0 +1,118 @@
+// RUN: %check_clang_tidy %s llvm-avoid-cast-in-conditional %t
+
+struct X;
+struct Y;
+struct Z {
+  int foo();
+  X *bar();
+};
+
+template 
+bool isa(Y *);
+template 
+X *cast(Y *);
+template 
+X *dyn_cast(Y *);
+template 
+X *dyn_cast_or_null(Y *);
+
+bool foo(Y *y, Z *z) {
+  if (auto x = cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:16: warning: {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (auto x = dyn_cast(y))
+
+  while (auto x = cast(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:19: warning:  {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (auto x = dyn_cast(y))
+
+  if (cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (isa(y))
+
+  while (cast(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:10: warning:  {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (isa(y))
+
+  do {
+break;
+  } while (cast(y));
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning:  {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (isa(y));
+
+  if (dyn_cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{.*dyn_cast<> not used .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (isa(y))
+
+  while (dyn_cast(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:10: warning:  {{.*dyn_cast<> not used .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (isa(y))
+
+  do {
+break;
+  } while (dyn_cast(y));
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning:  {{.*dyn_cast<> not used .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (isa(y));
+
+  if (z->bar() && isa(z->bar()))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{method .* is called twice .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (dyn_cast_or_null(z->bar()))
+
+  if (z->bar() && cast(z->bar()))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{method .* is called twice .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (dyn_cast_or_null(z->bar()))
+
+  if (z->bar() && dyn_cast(z->bar()))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{method .* is called twice .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (dyn_cast_or_null(z->bar()))
+
+  if (z->bar() && dyn_cast_or_null(z->bar()))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{method .* is called twice .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (dyn_cast_or_null(z->bar()))
+
+  bool b = z->bar() && cast(z->bar());
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning:  {{method .* is called twice .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: bool b = dyn_cast_or_null(z->bar());
+
+  // These don't trigger a warning.
+  if (auto x = cast(y)->foo())
+return true;
+  while (auto x = cast(y)->foo())
+break;
+  if (cast(y)->foo())
+return true;
+  while (cast(y)->foo())
+break;
+  if (y && isa(y))
+return true;
+  if (y && cast(z->bar()))
+return true;
+  if (z && cast(y)->foo())
+return true;
+  bool b2 = y && cast(z);
+
+#define CAST(T, Obj) cast(Obj)
+#define AUTO_VAR_CAST(X, Y, Z) auto X = cast(Z)
+#define ISA(T, Obj) isa(Obj)
+#define ISA_OR_NULL(T, Obj) Obj &(Obj)
+
+  if (auto x = CAST(X, y))
+return true;
+  if (AUTO_VAR_CAST(x, X, z))
+return true;
+  if (z->bar() && ISA(Y, z->bar()))
+return true;
+  if (ISA_OR_NULL(Y, z->bar()))
+return true;
+
+  return false;
+}
Index: clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
@@ -0,0 +1,28 @@
+.. title:: clang-tidy - llvm-avoid-cast-in-conditional
+

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

hintonda updated this revision to Diff 192854.
hintonda added a comment.

- Improve warning message.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802

Files:
  clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp
  clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.h
  clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
  clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
  clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp

Index: clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
@@ -0,0 +1,118 @@
+// RUN: %check_clang_tidy %s llvm-avoid-cast-in-conditional %t
+
+#define CAST(T, Obj) cast(Obj)
+#define ISA(T, Obj) isa(Obj)
+#define ISA_OR_NULL(T, Obj) Obj &(Obj)
+
+struct X;
+struct Y;
+struct Z {
+  int foo();
+  X *bar();
+};
+
+template 
+bool isa(Y *);
+template 
+X *cast(Y *);
+template 
+X *dyn_cast(Y *);
+template 
+X *dyn_cast_or_null(Y *);
+
+bool foo(Y *y, Z *z) {
+  if (auto x = cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:16: warning: {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (auto x = dyn_cast(y))
+
+  if (cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (isa(y))
+
+  if (dyn_cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{.*dyn_cast<> not used .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (isa(y))
+
+  // These don't trigger a warning.
+  if (auto x = cast(y)->foo())
+return true;
+  if (cast(y)->foo())
+return true;
+
+  while (auto x = cast(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:19: warning:  {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (auto x = dyn_cast(y))
+
+  while (cast(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:10: warning:  {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (isa(y))
+
+  while (dyn_cast(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:10: warning:  {{.*dyn_cast<> not used .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (isa(y))
+
+  // These don't trigger a warning.
+  while (auto x = cast(y)->foo())
+break;
+  while (cast(y)->foo())
+break;
+
+  do {
+break;
+  } while (cast(y));
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning:  {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (isa(y));
+
+  do {
+break;
+  } while (dyn_cast(y));
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning:  {{.*dyn_cast<> not used .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (isa(y));
+
+  if (z->bar() && isa(z->bar()))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{method .* is called twice .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (dyn_cast_or_null(z->bar()))
+
+  if (z->bar() && cast(z->bar()))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{method .* is called twice .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (dyn_cast_or_null(z->bar()))
+
+  if (z->bar() && dyn_cast(z->bar()))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{method .* is called twice .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (dyn_cast_or_null(z->bar()))
+
+  if (z->bar() && dyn_cast_or_null(z->bar()))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{method .* is called twice .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (dyn_cast_or_null(z->bar()))
+
+  bool b = z->bar() && cast(z->bar());
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning:  {{method .* is called twice .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: bool b = dyn_cast_or_null(z->bar());
+
+  // These don't trigger a warning.
+  if (auto x = CAST(X, y))
+return true;
+  if (z->bar() && ISA(Y, z->bar()))
+return true;
+  if (ISA_OR_NULL(Y, z->bar()))
+return true;
+  if (y && isa(y))
+return true;
+  if (y && cast(z->bar()))
+return true;
+  if (z && cast(y)->foo())
+return true;
+  bool b2 = y && cast(z);
+
+  return false;
+}
Index: clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
@@ -0,0 +1,28 @@
+.. title:: clang-tidy - llvm-avoid-cast-in-conditional
+
+llvm-avoid-cast-in-conditional

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

hintonda added a comment.

In D59802#1447560 , @aaron.ballman 
wrote:

> In D59802#1445566 , @hintonda wrote:
>
> > I looked at the IR generated at `-O2`, and found that while `if 
> > (isa(y))` is a modest win over `if (dyn_cast(y)`,  `if 
> > (dyn_cast_or_null(y))` generates exactly the same IR that `if(y && 
> > isa(y))` does.  Also, if `y` is actually an expression that makes a 
> > function call, it's more expensive because it will make the call twice.
> >
> > So I don't seen any reason to replace `dyn_cast_or_null<>` in conditionals.
>
>
> Mostly because I think it's more clear with `isa<>` because that's really 
> what it's checking. However, I could see not doing an automatic transform in 
> the case where the expression argument is something expensive, like a 
> function call. I could also see this as an impetus for adding `isa_or_null<>` 
> as a separate commit.


My last patch only changes `if(y && isa(y))` if `y` is a 
`CXXMemberCallExpr`, so I hope that addresses your concern.




Comment at: 
clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:145
+
+diag(MatchedDecl->getBeginLoc(), "use dyn_cast_or_null")
+<< FixItHint::CreateReplacement(SourceRange(MatchedDecl->getBeginLoc(),

aaron.ballman wrote:
> This diagnostic doesn't tell the user what they've done wrong with the code 
> or why this is a better choice.
Yes, but I'm not yet sure what it should say.  Was sorta hoping for a 
suggestion.  


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

hintonda updated this revision to Diff 192844.
hintonda marked 3 inline comments as done.
hintonda added a comment.

- Address comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802

Files:
  clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp
  clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.h
  clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
  clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
  clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp

Index: clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
@@ -0,0 +1,118 @@
+// RUN: %check_clang_tidy %s llvm-avoid-cast-in-conditional %t
+
+#define CAST(T, Obj) cast(Obj)
+#define ISA(T, Obj) isa(Obj)
+#define ISA_OR_NULL(T, Obj) Obj && isa(Obj)
+
+struct X;
+struct Y;
+struct Z {
+  int foo();
+  X *bar();
+};
+
+template 
+bool isa(Y *);
+template 
+X *cast(Y *);
+template 
+X *dyn_cast(Y *);
+template 
+X *dyn_cast_or_null(Y *);
+
+bool foo(Y *y, Z *z) {
+  if (auto x = cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:16: warning: {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (auto x = dyn_cast(y))
+
+  if (cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (isa(y))
+
+  if (dyn_cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{.*dyn_cast<> not used .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (isa(y))
+
+  // These don't trigger a warning.
+  if (auto x = cast(y)->foo())
+return true;
+  if (cast(y)->foo())
+return true;
+
+  while (auto x = cast(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:19: warning:  {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (auto x = dyn_cast(y))
+
+  while (cast(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:10: warning:  {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (isa(y))
+
+  while (dyn_cast(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:10: warning:  {{.*dyn_cast<> not used .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (isa(y))
+
+  // These don't trigger a warning.
+  while (auto x = cast(y)->foo())
+break;
+  while (cast(y)->foo())
+break;
+
+  do {
+break;
+  } while (cast(y));
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning:  {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (isa(y));
+
+  do {
+break;
+  } while (dyn_cast(y));
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning:  {{.*dyn_cast<> not used .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (isa(y));
+
+  if (z->bar() && isa(z->bar()))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{.*use dyn_cast_or_null .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (dyn_cast_or_null(z->bar()))
+
+  if (z->bar() && cast(z->bar()))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{.*use dyn_cast_or_null .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (dyn_cast_or_null(z->bar()))
+
+  if (z->bar() && dyn_cast(z->bar()))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{.*use dyn_cast_or_null .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (dyn_cast_or_null(z->bar()))
+
+  if (z->bar() && dyn_cast_or_null(z->bar()))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{.*use dyn_cast_or_null .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (dyn_cast_or_null(z->bar()))
+
+  bool b = z->bar() && cast(z->bar());
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning:  {{.*use dyn_cast_or_null .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: bool b = dyn_cast_or_null(z->bar());
+
+  // These don't trigger a warning.
+  if (auto x = CAST(X, y))
+return true;
+  if (z->bar() && ISA(Y, z->bar()))
+return true;
+  if (ISA_OR_NULL(Y, z->bar()))
+return true;
+  if (y && isa(y))
+return true;
+  if (y && cast(z->bar()))
+return true;
+  if (z && cast(y)->foo())
+return true;
+  bool b2 = y && cast(z);
+
+  return false;
+}
Index: clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
@@ -0,0 +1,28 @@
+.. title:: clang-tidy - llvm-avoid-cast-in-conditional
+

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked an inline comment as done.
aaron.ballman added a comment.

In D59802#1445566 , @hintonda wrote:

> I looked at the IR generated at `-O2`, and found that while `if (isa(y))` 
> is a modest win over `if (dyn_cast(y)`,  `if (dyn_cast_or_null(y))` 
> generates exactly the same IR that `if(y && isa(y))` does.  Also, if `y` 
> is actually an expression that makes a function call, it's more expensive 
> because it will make the call twice.
>
> So I don't seen any reason to replace `dyn_cast_or_null<>` in conditionals.


Mostly because I think it's more clear with `isa<>` because that's really what 
it's checking. However, I could see not doing an automatic transform in the 
case where the expression argument is something expensive, like a function 
call. I could also see this as an impetus for adding `isa_or_null<>` as a 
separate commit.




Comment at: clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:73
+  Result.Nodes.getNodeAs("cast-assign")) {
+auto StartLoc = MatchedDecl->getCallee()->getExprLoc();
+auto EndLoc = StartLoc.getLocWithOffset(StringRef("cast").size() - 1);

hintonda wrote:
> Eugene.Zelenko wrote:
> > Please don't use auto where type is not obvious.
> Happy to make the change, but thought the `get.*Loc()` methods were obvious?
Generally, no. There are a fair number of different location types and it's not 
obvious which type you're getting. We usually mean obvious as in "this type is 
an iterator, but the concrete type of the iterator isn't that important" or 
"the type is exactly spelled out as a template argument".



Comment at: clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:38
+  whileStmt(anyOf(
+  has(declStmt(containsDeclaration(
+  0, varDecl(

This seems like a fair amount of code duplication -- can this be cleaned up 
using some local variables for the inner matchers?



Comment at: clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:60
+  binaryOperator(
+  allOf(hasOperatorName("&&"), hasLHS(expr().bind("lhs")),
+hasRHS(anyOf(

I think the `expr()` matcher is too general. This will trigger on something 
like `if (foo && isa(bar))` won't it?

I'd also like to see tests for code like:
```
foo && cast(foo)->bar();
```
I don't think this pattern should diagnose because you cannot safely replace it 
with `dyn_cast(foo)->bar()`



Comment at: 
clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:62-64
+callExpr(
+allOf(hasArgument(0, expr().bind("arg")),
+  callee(namedDecl(hasName("isa")).bind("func"

I do not think we should diagnose code that uses `obj && isa(obj)` -- that's 
very reasonable code.



Comment at: 
clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:90-91
+ "cast<> in conditional will assert rather than return a null pointer")
+<< FixItHint::CreateReplacement(SourceRange(StartLoc, EndLoc),
+"dyn_cast");
+  } else if (const auto *MatchedDecl =

Should we be concerned about fix-it interactions with macros (here and 
elsewhere in the check)? We usually suppress fix-its if the replacement range 
is somewhere within a macro expansion.



Comment at: clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:96
+SourceLocation EndLoc =
+StartLoc.getLocWithOffset(StringRef("cast").size() - 1);
+

What if the reason we matched was because the `cast` was written using a macro 
of a different name? e.g., `#define LONGER_IDENTIFIER(T, Obj) cast(Obj)`



Comment at: 
clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:112-116
+// Don't touch Casting.h, since it would math otherwise.
+const StringRef Casting("llvm/include/llvm/Support/Casting.h");
+if (Result.SourceManager->getFilename(MatchedDecl->getBeginLoc())
+.take_back(Casting.size()) == Casting)
+  return;

Can this be handled at the matcher level? IIRC we have 
`isExpansionInFileMatching()` or something like that.



Comment at: 
clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:145
+
+diag(MatchedDecl->getBeginLoc(), "use dyn_cast_or_null")
+<< FixItHint::CreateReplacement(SourceRange(MatchedDecl->getBeginLoc(),

This diagnostic doesn't tell the user what they've done wrong with the code or 
why this is a better choice.



Comment at: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py:256-258
+  if args.fix:
+check_clang_apply_replacements_binary(args)
+

Is this related to your patch? If so, why is it needed?

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

hintonda updated this revision to Diff 192778.
hintonda added a comment.

- Add isa and dyn_cast when matching for dyn_cast_or_null replacement.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802

Files:
  clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp
  clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.h
  clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
  clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
  clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
  clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
  clang/include/clang/Basic/LLVM.h

Index: clang/include/clang/Basic/LLVM.h
===
--- clang/include/clang/Basic/LLVM.h
+++ clang/include/clang/Basic/LLVM.h
@@ -31,6 +31,7 @@
   template class ArrayRef;
   template class MutableArrayRef;
   template class OwningArrayRef;
+  template  class SmallSet;
   template class SmallString;
   template class SmallVector;
   template class SmallVectorImpl;
@@ -66,6 +67,7 @@
   using llvm::Optional;
   using llvm::OwningArrayRef;
   using llvm::SaveAndRestore;
+  using llvm::SmallSet;
   using llvm::SmallString;
   using llvm::SmallVector;
   using llvm::SmallVectorImpl;
Index: clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
@@ -0,0 +1,106 @@
+// RUN: %check_clang_tidy %s llvm-avoid-cast-in-conditional %t
+
+struct X;
+struct Y;
+struct Z {
+  int foo();
+  X *bar();
+};
+
+template 
+bool isa(Y *);
+template 
+X *cast(Y *);
+template 
+X *dyn_cast(Y *);
+template 
+X *dyn_cast_or_null(Y *);
+
+bool foo(Y *y) {
+  if (auto x = cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:16: warning: {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (auto x = dyn_cast(y))
+
+  if (cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (isa(y))
+
+  if (dyn_cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{.*dyn_cast<> not used .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (isa(y))
+
+  // These don't trigger a warning.
+  if (auto x = cast(y)->foo())
+return true;
+  if (cast(y)->foo())
+return true;
+
+  while (auto x = cast(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:19: warning:  {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (auto x = dyn_cast(y))
+
+  while (cast(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:10: warning:  {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (isa(y))
+
+  while (dyn_cast(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:10: warning:  {{.*dyn_cast<> not used .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (isa(y))
+
+  // These don't trigger a warning.
+  while (auto x = cast(y)->foo())
+break;
+  while (cast(y)->foo())
+break;
+
+  do {
+break;
+  } while (cast(y));
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning:  {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (isa(y));
+
+  do {
+break;
+  } while (dyn_cast(y));
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning:  {{.*dyn_cast<> not used .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (isa(y));
+
+  if (y && isa(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{.*use dyn_cast_or_null .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (dyn_cast_or_null(y))
+
+  if (y && cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{.*use dyn_cast_or_null .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (dyn_cast_or_null(y))
+
+  if (y && dyn_cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{.*use dyn_cast_or_null .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (dyn_cast_or_null(y))
+
+  Z z;
+  if (z.bar() && cast(z.bar()))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{.*use dyn_cast_or_null .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (dyn_cast_or_null(z.bar()))
+
+  bool b = y && cast(y);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning:  {{.*use dyn_cast_or_null .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: bool b = dyn_cast_or_null(y);
+
+  // These don't trigger a warning.
+  if (y && cast(z.bar())) {
+return true;
+  }
+  bool b2 = y && cast();
+
+  return false;
+}
Index: 

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

hintonda added a comment.

If the `dyn_cast_or_null<>` replacement is acceptable, I'll update the 
documentation and warning message -- suggestions are appreciated.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

hintonda updated this revision to Diff 192761.
hintonda added a comment.

- Remove spurious auto's.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802

Files:
  clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp
  clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.h
  clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
  clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
  clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
  clang/include/clang/Basic/LLVM.h

Index: clang/include/clang/Basic/LLVM.h
===
--- clang/include/clang/Basic/LLVM.h
+++ clang/include/clang/Basic/LLVM.h
@@ -31,6 +31,7 @@
   template class ArrayRef;
   template class MutableArrayRef;
   template class OwningArrayRef;
+  template  class SmallSet;
   template class SmallString;
   template class SmallVector;
   template class SmallVectorImpl;
@@ -66,6 +67,7 @@
   using llvm::Optional;
   using llvm::OwningArrayRef;
   using llvm::SaveAndRestore;
+  using llvm::SmallSet;
   using llvm::SmallString;
   using llvm::SmallVector;
   using llvm::SmallVectorImpl;
Index: clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
@@ -0,0 +1,96 @@
+// RUN: %check_clang_tidy %s llvm-avoid-cast-in-conditional %t
+
+struct X;
+struct Y;
+struct Z {
+  int foo();
+  X *bar();
+};
+
+template 
+bool isa(Y *);
+template 
+X *cast(Y *);
+template 
+X *dyn_cast(Y *);
+template 
+X *dyn_cast_or_null(Y *);
+
+bool foo(Y *y) {
+  if (auto x = cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:16: warning: {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (auto x = dyn_cast(y))
+
+  if (cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (isa(y))
+
+  if (dyn_cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{.*dyn_cast<> not used .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (isa(y))
+
+  // These don't trigger a warning.
+  if (auto x = cast(y)->foo())
+return true;
+  if (cast(y)->foo())
+return true;
+
+  while (auto x = cast(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:19: warning:  {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (auto x = dyn_cast(y))
+
+  while (cast(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:10: warning:  {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (isa(y))
+
+  while (dyn_cast(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:10: warning:  {{.*dyn_cast<> not used .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (isa(y))
+
+  // These don't trigger a warning.
+  while (auto x = cast(y)->foo())
+break;
+  while (cast(y)->foo())
+break;
+
+  do {
+break;
+  } while (cast(y));
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning:  {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (isa(y));
+
+  do {
+break;
+  } while (dyn_cast(y));
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning:  {{.*dyn_cast<> not used .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (isa(y));
+
+  if (y && cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{.*use dyn_cast_or_null .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (dyn_cast_or_null(y))
+
+  Z z;
+  if (z.bar() && cast(z.bar()))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{.*use dyn_cast_or_null .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (dyn_cast_or_null(z.bar()))
+
+  bool b = y && cast(y);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning:  {{.*use dyn_cast_or_null .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: bool b = dyn_cast_or_null(y);
+
+  // These don't trigger a warning.
+  if (y && cast(z.bar())) {
+return true;
+  }
+  bool b2 = y && cast();
+
+  return false;
+}
Index: clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
@@ -0,0 +1,28 @@
+.. title:: clang-tidy - llvm-avoid-cast-in-conditional
+
+llvm-avoid-cast-in-conditional
+==
+
+  Finds uses of ``cast<>`` in conditionals of ``if``, ``while`` or
+  ``do`` statements, which will assert rather than return a null
+  pointer. It 

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

hintonda added a comment.

In D59802#1447127 , @Eugene.Zelenko 
wrote:

> In D59802#1447108 , @hintonda wrote:
>
> >
>
>
> It's good idea to follow modernize-use-auto convention without introducing 
> exceptions.


No worries.  I'm just not sure I fully grok them.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In D59802#1447108 , @hintonda wrote:

>


It's good idea to follow modernize-use-auto convention without introducing 
exceptions.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

hintonda marked an inline comment as done.
hintonda added a comment.






Comment at: clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:73
+  Result.Nodes.getNodeAs("cast-assign")) {
+auto StartLoc = MatchedDecl->getCallee()->getExprLoc();
+auto EndLoc = StartLoc.getLocWithOffset(StringRef("cast").size() - 1);

Eugene.Zelenko wrote:
> Please don't use auto where type is not obvious.
Happy to make the change, but thought the `get.*Loc()` methods were obvious?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:73
+  Result.Nodes.getNodeAs("cast-assign")) {
+auto StartLoc = MatchedDecl->getCallee()->getExprLoc();
+auto EndLoc = StartLoc.getLocWithOffset(StringRef("cast").size() - 1);

Please don't use auto where type is not obvious.



Comment at: clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:74
+auto StartLoc = MatchedDecl->getCallee()->getExprLoc();
+auto EndLoc = StartLoc.getLocWithOffset(StringRef("cast").size() - 1);
+

Please don't use auto where type is not obvious.



Comment at: clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:82
+ Result.Nodes.getNodeAs("cast")) {
+auto StartLoc = MatchedDecl->getCallee()->getExprLoc();
+auto EndLoc = StartLoc.getLocWithOffset(StringRef("cast").size() - 1);

Please don't use auto where type is not obvious.



Comment at: clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:83
+auto StartLoc = MatchedDecl->getCallee()->getExprLoc();
+auto EndLoc = StartLoc.getLocWithOffset(StringRef("cast").size() - 1);
+

Please don't use auto where type is not obvious.



Comment at: clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:90
+ Result.Nodes.getNodeAs("dyn_cast")) {
+auto StartLoc = MatchedDecl->getCallee()->getExprLoc();
+auto EndLoc = StartLoc.getLocWithOffset(StringRef("dyn_cast").size() - 1);

Please don't use auto where type is not obvious.



Comment at: clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:91
+auto StartLoc = MatchedDecl->getCallee()->getExprLoc();
+auto EndLoc = StartLoc.getLocWithOffset(StringRef("dyn_cast").size() - 1);
+

Please don't use auto where type is not obvious.



Comment at: 
clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:101
+
+auto ArgRange = CharSourceRange::getTokenRange(Arg->getSourceRange());
+std::string ArgString(

Please don't use auto where type is not obvious.



Comment at: 
clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:104
+Lexer::getSourceText(ArgRange, *Result.SourceManager, getLangOpts()));
+auto LHSRange = CharSourceRange::getTokenRange(LHS->getSourceRange());
+std::string LHSString(

Please don't use auto where type is not obvious.



Comment at: 
clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:107
+Lexer::getSourceText(LHSRange, *Result.SourceManager, getLangOpts()));
+auto RHSRange = CharSourceRange::getTokenRange(RHS->getSourceRange());
+std::string RHSString(

Please don't use auto where type is not obvious.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

hintonda updated this revision to Diff 192756.
hintonda added a comment.

- Remove commented code.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802

Files:
  clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp
  clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.h
  clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
  clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
  clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
  clang/include/clang/Basic/LLVM.h

Index: clang/include/clang/Basic/LLVM.h
===
--- clang/include/clang/Basic/LLVM.h
+++ clang/include/clang/Basic/LLVM.h
@@ -31,6 +31,7 @@
   template class ArrayRef;
   template class MutableArrayRef;
   template class OwningArrayRef;
+  template  class SmallSet;
   template class SmallString;
   template class SmallVector;
   template class SmallVectorImpl;
@@ -66,6 +67,7 @@
   using llvm::Optional;
   using llvm::OwningArrayRef;
   using llvm::SaveAndRestore;
+  using llvm::SmallSet;
   using llvm::SmallString;
   using llvm::SmallVector;
   using llvm::SmallVectorImpl;
Index: clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
@@ -0,0 +1,96 @@
+// RUN: %check_clang_tidy %s llvm-avoid-cast-in-conditional %t
+
+struct X;
+struct Y;
+struct Z {
+  int foo();
+  X *bar();
+};
+
+template 
+bool isa(Y *);
+template 
+X *cast(Y *);
+template 
+X *dyn_cast(Y *);
+template 
+X *dyn_cast_or_null(Y *);
+
+bool foo(Y *y) {
+  if (auto x = cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:16: warning: {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (auto x = dyn_cast(y))
+
+  if (cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (isa(y))
+
+  if (dyn_cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{.*dyn_cast<> not used .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (isa(y))
+
+  // These don't trigger a warning.
+  if (auto x = cast(y)->foo())
+return true;
+  if (cast(y)->foo())
+return true;
+
+  while (auto x = cast(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:19: warning:  {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (auto x = dyn_cast(y))
+
+  while (cast(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:10: warning:  {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (isa(y))
+
+  while (dyn_cast(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:10: warning:  {{.*dyn_cast<> not used .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (isa(y))
+
+  // These don't trigger a warning.
+  while (auto x = cast(y)->foo())
+break;
+  while (cast(y)->foo())
+break;
+
+  do {
+break;
+  } while (cast(y));
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning:  {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (isa(y));
+
+  do {
+break;
+  } while (dyn_cast(y));
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning:  {{.*dyn_cast<> not used .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (isa(y));
+
+  if (y && cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{.*use dyn_cast_or_null .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (dyn_cast_or_null(y))
+
+  Z z;
+  if (z.bar() && cast(z.bar()))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{.*use dyn_cast_or_null .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (dyn_cast_or_null(z.bar()))
+
+  bool b = y && cast(y);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning:  {{.*use dyn_cast_or_null .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: bool b = dyn_cast_or_null(y);
+
+  // These don't trigger a warning.
+  if (y && cast(z.bar())) {
+return true;
+  }
+  bool b2 = y && cast();
+
+  return false;
+}
Index: clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
@@ -0,0 +1,28 @@
+.. title:: clang-tidy - llvm-avoid-cast-in-conditional
+
+llvm-avoid-cast-in-conditional
+==
+
+  Finds uses of ``cast<>`` in conditionals of ``if``, ``while`` or
+  ``do`` statements, which will assert rather than return a null
+  pointer. It 

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

hintonda updated this revision to Diff 192754.
hintonda added a comment.

- Add replacement of `y && cast(y)` with `dyn_cast_or_null(y)`, which is 
at least as efficient, and can be a big win if `y` is a function call.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802

Files:
  clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp
  clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.h
  clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
  clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
  clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
  clang/include/clang/Basic/LLVM.h

Index: clang/include/clang/Basic/LLVM.h
===
--- clang/include/clang/Basic/LLVM.h
+++ clang/include/clang/Basic/LLVM.h
@@ -31,6 +31,7 @@
   template class ArrayRef;
   template class MutableArrayRef;
   template class OwningArrayRef;
+  template  class SmallSet;
   template class SmallString;
   template class SmallVector;
   template class SmallVectorImpl;
@@ -66,6 +67,7 @@
   using llvm::Optional;
   using llvm::OwningArrayRef;
   using llvm::SaveAndRestore;
+  using llvm::SmallSet;
   using llvm::SmallString;
   using llvm::SmallVector;
   using llvm::SmallVectorImpl;
Index: clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
@@ -0,0 +1,96 @@
+// RUN: %check_clang_tidy %s llvm-avoid-cast-in-conditional %t
+
+struct X;
+struct Y;
+struct Z {
+  int foo();
+  X *bar();
+};
+
+template 
+bool isa(Y *);
+template 
+X *cast(Y *);
+template 
+X *dyn_cast(Y *);
+template 
+X *dyn_cast_or_null(Y *);
+
+bool foo(Y *y) {
+  if (auto x = cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:16: warning: {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (auto x = dyn_cast(y))
+
+  if (cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (isa(y))
+
+  if (dyn_cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{.*dyn_cast<> not used .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (isa(y))
+
+  // These don't trigger a warning.
+  if (auto x = cast(y)->foo())
+return true;
+  if (cast(y)->foo())
+return true;
+
+  while (auto x = cast(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:19: warning:  {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (auto x = dyn_cast(y))
+
+  while (cast(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:10: warning:  {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (isa(y))
+
+  while (dyn_cast(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:10: warning:  {{.*dyn_cast<> not used .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (isa(y))
+
+  // These don't trigger a warning.
+  while (auto x = cast(y)->foo())
+break;
+  while (cast(y)->foo())
+break;
+
+  do {
+break;
+  } while (cast(y));
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning:  {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (isa(y));
+
+  do {
+break;
+  } while (dyn_cast(y));
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning:  {{.*dyn_cast<> not used .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (isa(y));
+
+  if (y && cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{.*use dyn_cast_or_null .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (dyn_cast_or_null(y))
+
+  Z z;
+  if (z.bar() && cast(z.bar()))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{.*use dyn_cast_or_null .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (dyn_cast_or_null(z.bar()))
+
+  bool b = y && cast(y);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning:  {{.*use dyn_cast_or_null .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: bool b = dyn_cast_or_null(y);
+
+  // These don't trigger a warning.
+  if (y && cast(z.bar())) {
+return true;
+  }
+  bool b2 = y && cast();
+
+  return false;
+}
Index: clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
@@ -0,0 +1,28 @@
+.. title:: clang-tidy - llvm-avoid-cast-in-conditional
+
+llvm-avoid-cast-in-conditional
+==
+
+  Finds uses of ``cast<>`` in 

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

hintonda updated this revision to Diff 192577.
hintonda added a comment.

- Removed the dyn_cast_or_null replacements.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802

Files:
  clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp
  clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.h
  clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
  clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
  clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
  clang/include/clang/Basic/LLVM.h

Index: clang/include/clang/Basic/LLVM.h
===
--- clang/include/clang/Basic/LLVM.h
+++ clang/include/clang/Basic/LLVM.h
@@ -31,6 +31,7 @@
   template class ArrayRef;
   template class MutableArrayRef;
   template class OwningArrayRef;
+  template  class SmallSet;
   template class SmallString;
   template class SmallVector;
   template class SmallVectorImpl;
@@ -66,6 +67,7 @@
   using llvm::Optional;
   using llvm::OwningArrayRef;
   using llvm::SaveAndRestore;
+  using llvm::SmallSet;
   using llvm::SmallString;
   using llvm::SmallVector;
   using llvm::SmallVectorImpl;
Index: clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
@@ -0,0 +1,74 @@
+// RUN: %check_clang_tidy %s llvm-avoid-cast-in-conditional %t
+
+struct X;
+struct Y;
+struct Z {
+  int foo();
+};
+
+template 
+bool isa(Y *);
+template 
+X *cast(Y *);
+template 
+X *dyn_cast(Y *);
+template 
+X *dyn_cast_or_null(Y *);
+
+bool foo(Y *y) {
+  if (auto x = cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:16: warning: {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (auto x = dyn_cast(y))
+
+  if (cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (isa(y))
+
+  if (dyn_cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{.*dyn_cast<> not used .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (isa(y))
+
+  // These don't trigger a warning.
+  if (auto x = cast(y)->foo())
+return true;
+  if (cast(y)->foo())
+return true;
+
+  while (auto x = cast(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:19: warning:  {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (auto x = dyn_cast(y))
+
+  while (cast(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:10: warning:  {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (isa(y))
+
+  while (dyn_cast(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:10: warning:  {{.*dyn_cast<> not used .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (isa(y))
+
+  // These don't trigger a warning.
+  while (auto x = cast(y)->foo())
+break;
+  while (cast(y)->foo())
+break;
+
+  do {
+break;
+  } while (cast(y));
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning:  {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (isa(y));
+
+  do {
+break;
+  } while (dyn_cast(y));
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning:  {{.*dyn_cast<> not used .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (isa(y));
+
+  return false;
+}
Index: clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
@@ -0,0 +1,28 @@
+.. title:: clang-tidy - llvm-avoid-cast-in-conditional
+
+llvm-avoid-cast-in-conditional
+==
+
+  Finds uses of ``cast<>`` in conditionals of ``if``, ``while`` or
+  ``do`` statements, which will assert rather than return a null
+  pointer. It also finds uses of ``dyn_cast<>`` in conditionals where
+  the return value is not captured.
+
+.. code-block:: c++
+
+  // Finds these:
+  if (auto x = cast(y)) {}
+  // is replaced by:
+  if (auto x = dyn_cast(y)) {}
+
+  if (cast(y)) {}
+  // is replaced by:
+  if (isa(y)) {}
+
+  if (dyn_cast(y)) {}
+  // is replaced by:
+  if (isa(y)) {}
+
+  // These are ignored.
+  if (auto f = cast(y)->foo()) {}
+  if (cast(y)->foo()) {}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -175,6 +175,7 @@
hicpp-use-nullptr 

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

hintonda added a comment.

I looked at the IR generated at `-O2`, and found that while `if (isa(y))` is 
a modest win over `if (dyn_cast(y)`,  `if (dyn_cast_or_null(y))` 
generates exactly the same IR that `if(y && isa(y))` does.  Also, if `y` is 
actually an expression that makes a function call, it's more expensive because 
it will make the call twice.

So I don't seen any reason to replace `dyn_cast_or_null<>` in conditionals.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

hintonda updated this revision to Diff 192416.
hintonda added a comment.

- Added dyn_cast and dyn_cast_or_null, and provided fixit's for all of them.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802

Files:
  clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp
  clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.h
  clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
  clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
  clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
  clang/include/clang/Basic/LLVM.h

Index: clang/include/clang/Basic/LLVM.h
===
--- clang/include/clang/Basic/LLVM.h
+++ clang/include/clang/Basic/LLVM.h
@@ -31,6 +31,7 @@
   template class ArrayRef;
   template class MutableArrayRef;
   template class OwningArrayRef;
+  template  class SmallSet;
   template class SmallString;
   template class SmallVector;
   template class SmallVectorImpl;
@@ -66,6 +67,7 @@
   using llvm::Optional;
   using llvm::OwningArrayRef;
   using llvm::SaveAndRestore;
+  using llvm::SmallSet;
   using llvm::SmallString;
   using llvm::SmallVector;
   using llvm::SmallVectorImpl;
Index: clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
@@ -0,0 +1,102 @@
+// RUN: %check_clang_tidy %s llvm-avoid-cast-in-conditional %t
+
+struct X;
+struct Y;
+struct Z {
+  int foo();
+  X *bar();
+};
+
+template 
+bool isa(Y *);
+template 
+X *cast(Y *);
+template 
+X *dyn_cast(Y *);
+template 
+X *dyn_cast_or_null(Y *);
+
+bool foo(Y *y, Z *LongVarName) {
+  if (auto x = cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:16: warning: {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (auto x = dyn_cast(y))
+
+  if (cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (isa(y))
+
+  if (dyn_cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{.*dyn_cast<> not used .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (isa(y))
+
+  if (dyn_cast_or_null(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{.*dyn_cast_or_null<> not used.*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (y && isa(y))
+
+  if (dyn_cast_or_null(LongVarName->bar()))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{.*dyn_cast_or_null<> not used.*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (LongVarName->bar() && isa(LongVarName->bar()))
+
+  // These don't trigger a warning.
+  if (auto x = cast(y)->foo())
+return true;
+  if (cast(y)->foo())
+return true;
+
+  while (auto x = cast(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:19: warning:  {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (auto x = dyn_cast(y))
+
+  while (cast(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:10: warning:  {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (isa(y))
+
+  while (dyn_cast(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:10: warning:  {{.*dyn_cast<> not used .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (isa(y))
+
+  while (dyn_cast_or_null(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:10: warning:  {{.*dyn_cast_or_null<> not used .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (y && isa(y))
+
+  // These don't trigger a warning.
+  while (auto x = cast(y)->foo())
+break;
+  while (cast(y)->foo())
+break;
+
+  do {
+break;
+  } while (cast(y));
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning:  {{cast<> in conditional .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (isa(y));
+
+  do {
+break;
+  } while (dyn_cast(y));
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning:  {{.*dyn_cast<> not used .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (isa(y));
+
+  do {
+break;
+  } while (dyn_cast_or_null(y));
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning:  {{.*dyn_cast_or_null<> not used .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (y && isa(y))
+
+  do {
+break;
+  } while (dyn_cast_or_null(LongVarName->bar()));
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning:  {{.*dyn_cast_or_null<> not used .*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: while (LongVarName->bar() && isa(LongVarName->bar()))
+
+  return false;
+}
Index: 

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

hintonda updated this revision to Diff 192370.
hintonda added a comment.

- Address additional comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802

Files:
  clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp
  clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.h
  clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
  clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
  clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
  clang/include/clang/Basic/LLVM.h

Index: clang/include/clang/Basic/LLVM.h
===
--- clang/include/clang/Basic/LLVM.h
+++ clang/include/clang/Basic/LLVM.h
@@ -31,6 +31,7 @@
   template class ArrayRef;
   template class MutableArrayRef;
   template class OwningArrayRef;
+  template  class SmallSet;
   template class SmallString;
   template class SmallVector;
   template class SmallVectorImpl;
@@ -66,6 +67,7 @@
   using llvm::Optional;
   using llvm::OwningArrayRef;
   using llvm::SaveAndRestore;
+  using llvm::SmallSet;
   using llvm::SmallString;
   using llvm::SmallVector;
   using llvm::SmallVectorImpl;
Index: clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
@@ -0,0 +1,25 @@
+// RUN: %check_clang_tidy %s llvm-avoid-cast-in-conditional %t
+
+struct X;
+struct Y;
+struct Z {
+  int foo();
+};
+
+template 
+X* cast(Y*);
+
+bool foo(Y* y) {
+  if (auto x = cast(y))
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: {{.*llvm-avoid-cast-in-conditional}}
+return true;
+  if (cast(y))
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning:  {{.*llvm-avoid-cast-in-conditional}}
+return true;
+
+  // Does not trigger warning.
+  if (cast(y)->foo())
+return true;
+  return false;
+}
+
Index: clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
@@ -0,0 +1,19 @@
+.. title:: clang-tidy - llvm-avoid-cast-in-conditional
+
+llvm-avoid-cast-in-conditional
+==
+
+Finds cases of ``cast<>`` used as the condition of a conditional
+statements which will assert on failure in Debug builds. The use of
+``cast<>`` implies that the operation cannot fail, and should not be
+used as the condition of a conditional statement.
+
+.. code-block:: c++
+
+  // Finds cases like these:
+  if (auto x = cast(y)) <...>
+  if (cast(y)) <...>
+
+  // But not cases like these:
+  if (auto f = cast(y)->foo()) <...>
+  if (cast(y)->foo()) <...>
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -175,6 +175,7 @@
hicpp-use-nullptr (redirects to modernize-use-nullptr) 
hicpp-use-override (redirects to modernize-use-override) 
hicpp-vararg (redirects to cppcoreguidelines-pro-type-vararg) 
+   llvm-avoid-cast-in-conditional
llvm-header-guard
llvm-include-order
llvm-namespace-comment
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -130,6 +130,12 @@
   ` now supports `OverrideSpelling`
   and `FinalSpelling` options.
 
+- New :doc:`llvm-avoid-cast-in-conditional
+  ` check.
+
+  Finds cases of ``cast<>`` used as condition in conditional statements
+  which will assert on failure in Debug builds.
+
 - New :doc:`openmp-exception-escape
   ` check.
 
Index: clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
===
--- clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
+++ clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
@@ -18,7 +18,7 @@
 namespace tidy {
 namespace utils {
 
-typedef llvm::SmallSet HeaderFileExtensionsSet;
+using HeaderFileExtensionsSet = SmallSet;
 
 /// \brief Checks whether expansion location of \p Loc is in header file.
 bool isExpansionLocInHeaderFile(
Index: clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
===
--- clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
+++ clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
@@ -10,6 +10,7 @@
 #include "../ClangTidyModule.h"
 

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

hintonda added a comment.

In D59802#1443527 , @aaron.ballman 
wrote:

> Would it make sense to transform `if (dyn_cast_or_null(Obj))` into `if 
> (Obj && isa(Obj))`  or are there bad transformations from that?


Sure, that sounds reasonable.  I only see about 12 cases across all repos, so 
it isn't that common.  Whereas the idiom you present is used quite often.

I haven't looked yet, but wouldn't the use of `isa_or_null<>` be more efficient 
in cases like this?

./clang/lib/Sema/AnalysisBasedWarnings.cpp:401:  if (B->getTerminator() 
&& isa(B->getTerminator()))
./clang/lib/AST/Expr.cpp:2734:if (MCE->getMethodDecl() && 
isa(MCE->getMethodDecl()))

Granted, there aren't a lot of these.

> 
> 
>> Btw, I also found the same pattern used for `while()`, so I'll add that too. 
>>  Here's a sample of the patterns I'm seeing:
>> 
>> ./lldb/source/Plugins/ExpressionParser/Clang/ASTResultSynthesizer.cpp:213:  
>> while (dyn_cast(last_stmt)) {
> 
> Hah, good catch!
> 
>> ./clang/lib/CodeGen/CodeGenModule.cpp:1390:  if 
>> (dyn_cast_or_null(D)) . // <--- this one's okay
> 
> I think this could be expressed as `if (D && isa(D))`, no?




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D59802#1443515 , @hintonda wrote:

> In D59802#1443451 , @hintonda wrote:
>
> > In D59802#1443340 , @aaron.ballman 
> > wrote:
> >
> > > Should this check also try to find this pattern:
> > >
> > >   if (dyn_cast(Bobble))
> > > ...
> > >
> > >
> > > in order to recommend the proper replacement:
> > >
> > >   if (isa(Bobble))
> > > ...
> > >
> > >
> > > I ask because the name `llvm-avoid-cast-in-conditional` sounds like it 
> > > would also cover this situation and I run into it during code reviews 
> > > with some frequency (more often than I run into `cast<>` being used in a 
> > > conditional).
> >
> >
> > Yes, I can add that, and provide a fix-it too.  Thanks...
>
>
> I did a quick grep and found a few of these, but also found the `_or_null<>` 
> variety.  Guess they'll need to stay the same since `isa<>` can't handle 
> nulls.


Would it make sense to transform `if (dyn_cast_or_null(Obj))` into `if (Obj 
&& isa(Obj))`  or are there bad transformations from that?

> Btw, I also found the same pattern used for `while()`, so I'll add that too.  
> Here's a sample of the patterns I'm seeing:
> 
> ./lldb/source/Plugins/ExpressionParser/Clang/ASTResultSynthesizer.cpp:213:  
> while (dyn_cast(last_stmt)) {

Hah, good catch!

> ./clang/lib/CodeGen/CodeGenModule.cpp:1390:  if 
> (dyn_cast_or_null(D)) . // <--- this one's okay

I think this could be expressed as `if (D && isa(D))`, no?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

hintonda added a comment.

In D59802#1443451 , @hintonda wrote:

> In D59802#1443340 , @aaron.ballman 
> wrote:
>
> > Should this check also try to find this pattern:
> >
> >   if (dyn_cast(Bobble))
> > ...
> >
> >
> > in order to recommend the proper replacement:
> >
> >   if (isa(Bobble))
> > ...
> >
> >
> > I ask because the name `llvm-avoid-cast-in-conditional` sounds like it 
> > would also cover this situation and I run into it during code reviews with 
> > some frequency (more often than I run into `cast<>` being used in a 
> > conditional).
>
>
> Yes, I can add that, and provide a fix-it too.  Thanks...


I did a quick grep and found a few of these, but also found the `_or_null<>` 
variety.  Guess they'll need to stay the same since `isa<>` can't handle nulls.

Btw, I also found the same pattern used for `while()`, so I'll add that too.  
Here's a sample of the patterns I'm seeing:

./lldb/source/Plugins/ExpressionParser/Clang/ASTResultSynthesizer.cpp:213:  
while (dyn_cast(last_stmt)) {
./clang/lib/CodeGen/CodeGenModule.cpp:1390:  if 
(dyn_cast_or_null(D)) . // <--- this one's okay
./clang/lib/CodeGen/CodeGenModule.cpp:3950:if 
(dyn_cast(callSite)) {


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

hintonda added a comment.

In D59802#1443340 , @aaron.ballman 
wrote:

> Should this check also try to find this pattern:
>
>   if (dyn_cast(Bobble))
> ...
>
>
> in order to recommend the proper replacement:
>
>   if (isa(Bobble))
> ...
>
>
> I ask because the name `llvm-avoid-cast-in-conditional` sounds like it would 
> also cover this situation and I run into it during code reviews with some 
> frequency (more often than I run into `cast<>` being used in a conditional).


Yes, I can add that, and provide a fix-it too.  Thanks...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst:8
+statements which will assert on failure in Debug builds. The use of
+cast<> implies that the operation cannot fail, and should not be used
+as the condition of a conditional statement.

This cast<> still needs to be enclosed in ``.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Should this check also try to find this pattern:

  if (dyn_cast(Bobble))
...

in order to recommend the proper replacement:

  if (isa(Bobble))
...

I ask because the name `llvm-avoid-cast-in-conditional` sounds like it would 
also cover this situation and I run into it during code reviews with some 
frequency (more often than I run into `cast<>` being used in a conditional).




Comment at: clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:20
+void AvoidCastInConditionalCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+  ifStmt(anyOf(

No need to register this matcher if C++ isn't enabled.



Comment at: 
clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:35-36
+diag(MatchedDecl->getBeginLoc(),
+ "Found cast<> in conditional statement; consider using dyn_cast<> if "
+ "it can fail, or removal of conditional if it can't.");
+;

clang-tidy diagnostics should not be complete sentences; you should make this 
ungrammatical. How about: `cast<> in conditional will assert rather than return 
a null pointer`?



Comment at: clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:37
+ "it can fail, or removal of conditional if it can't.");
+;
+  }

Spurious semicolon?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

hintonda updated this revision to Diff 192238.
hintonda added a comment.

- Address additional comments, and move  to LLVM.h.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802

Files:
  clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp
  clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.h
  clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
  clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
  clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
  clang/include/clang/Basic/LLVM.h

Index: clang/include/clang/Basic/LLVM.h
===
--- clang/include/clang/Basic/LLVM.h
+++ clang/include/clang/Basic/LLVM.h
@@ -31,6 +31,7 @@
   template class ArrayRef;
   template class MutableArrayRef;
   template class OwningArrayRef;
+  template  class SmallSet;
   template class SmallString;
   template class SmallVector;
   template class SmallVectorImpl;
@@ -66,6 +67,7 @@
   using llvm::Optional;
   using llvm::OwningArrayRef;
   using llvm::SaveAndRestore;
+  using llvm::SmallSet;
   using llvm::SmallString;
   using llvm::SmallVector;
   using llvm::SmallVectorImpl;
Index: clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
@@ -0,0 +1,25 @@
+// RUN: %check_clang_tidy %s llvm-avoid-cast-in-conditional %t
+
+struct X;
+struct Y;
+struct Z {
+  int foo();
+};
+
+template 
+X* cast(Y*);
+
+bool foo(Y* y) {
+  if (auto x = cast(y))
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: {{.*llvm-avoid-cast-in-conditional}}
+return true;
+  if (cast(y))
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning:  {{.*llvm-avoid-cast-in-conditional}}
+return true;
+
+  // Does not trigger warning.
+  if (cast(y)->foo())
+return true;
+  return false;
+}
+
Index: clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
@@ -0,0 +1,19 @@
+.. title:: clang-tidy - llvm-avoid-cast-in-conditional
+
+llvm-avoid-cast-in-conditional
+==
+
+Finds cases of ``cast<>`` used as the condition of a conditional
+statements which will assert on failure in Debug builds. The use of
+cast<> implies that the operation cannot fail, and should not be used
+as the condition of a conditional statement.
+
+.. code-block:: c++
+
+  // Finds cases like these:
+  if (auto x = cast(y)) <...>
+  if (cast(y)) <...>
+
+  // But not cases like these:
+  if (auto f = cast(y)->foo()) <...>
+  if (cast(y)->foo()) <...>
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -175,6 +175,7 @@
hicpp-use-nullptr (redirects to modernize-use-nullptr) 
hicpp-use-override (redirects to modernize-use-override) 
hicpp-vararg (redirects to cppcoreguidelines-pro-type-vararg) 
+   llvm-avoid-cast-in-conditional
llvm-header-guard
llvm-include-order
llvm-namespace-comment
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -130,6 +130,12 @@
   ` now supports `OverrideSpelling`
   and `FinalSpelling` options.
 
+- New :doc:`llvm-avoid-cast-in-conditional
+  ` check.
+
+  Finds cases of ``cast<>`` used as condition in conditional statements
+  which will assert on failure in Debug builds.
+
 - New :doc:`openmp-exception-escape
   ` check.
 
Index: clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
===
--- clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
+++ clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
@@ -18,7 +18,7 @@
 namespace tidy {
 namespace utils {
 
-typedef llvm::SmallSet HeaderFileExtensionsSet;
+using HeaderFileExtensionsSet = SmallSet;
 
 /// \brief Checks whether expansion location of \p Loc is in header file.
 bool isExpansionLocInHeaderFile(
Index: clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
===
--- clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
+++ clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
@@ -10,6 +10,7 @@
 #include 

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h:17
 
+using llvm::SmallSet;
+

hintonda wrote:
> Eugene.Zelenko wrote:
> > It'll be easier to use llvm::SmallSet in using statement.
> Unfortunately, that won't compile.
> 
> The reason is that the llvm checkers live in `namespace 
> clang::clang-tidy::llvm`, so if you include an llvm checker header, which 
> opens the `clang::clang-tidy::llvm` namespace, clang doesn't know whether to 
> look in `llvm` or `clang::clang-tidy::llvm` since you are in 
> `clang::clang-tidy`.
> 
> The error does suggest using `::llvm::SmallSet`, but that seems sorta klugey.
> 
> Since this looks ugly, how about I add `using llvm::SmallSet;` to ` 
> clang/include/clang/LLVM.h`?
> 
Btw, the reason this never caused a problem before was that the first llvm 
checker header included in 
`clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp` was 
`HeaderGuardCheck.h`, which only include `HeaderGuardCheck.h` before opening 
the new `clang::clang-tidy::llvm` namespace.  Once I added this checker, which 
was included before `HeaderGuardCheck.h`, it made the new `llvm` namespace was 
ambiguous within `clang-tidy`, hence the error.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h:17
 
+using llvm::SmallSet;
+

Eugene.Zelenko wrote:
> It'll be easier to use llvm::SmallSet in using statement.
Unfortunately, that won't compile.

The reason is that the llvm checkers live in `namespace 
clang::clang-tidy::llvm`, so if you include an llvm checker header, which opens 
the `clang::clang-tidy::llvm` namespace, clang doesn't know whether to look in 
`llvm` or `clang::clang-tidy::llvm` since you are in `clang::clang-tidy`.

The error does suggest using `::llvm::SmallSet`, but that seems sorta klugey.

Since this looks ugly, how about I add `using llvm::SmallSet;` to ` 
clang/include/clang/LLVM.h`?



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h:17
 
+using llvm::SmallSet;
+

It'll be easier to use llvm::SmallSet in using statement.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:136
+
+  Finds cases of `cast<>` used as condition in conditional statements
+  which will assert on failure in Debug builds.

Please use double, not single `.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

hintonda updated this revision to Diff 192234.
hintonda added a comment.

- Address comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802

Files:
  clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp
  clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.h
  clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
  clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
  clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp

Index: clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
@@ -0,0 +1,25 @@
+// RUN: %check_clang_tidy %s llvm-avoid-cast-in-conditional %t
+
+struct X;
+struct Y;
+struct Z {
+  int foo();
+};
+
+template 
+X* cast(Y*);
+
+bool foo(Y* y) {
+  if (auto x = cast(y))
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: {{.*llvm-avoid-cast-in-conditional}}
+return true;
+  if (cast(y))
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning:  {{.*llvm-avoid-cast-in-conditional}}
+return true;
+
+  // Does not trigger warning.
+  if (cast(y)->foo())
+return true;
+  return false;
+}
+
Index: clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
@@ -0,0 +1,19 @@
+.. title:: clang-tidy - llvm-avoid-cast-in-conditional
+
+llvm-avoid-cast-in-conditional
+==
+
+Finds cases of `cast<>` used as the condition of a conditional
+statements which will assert on failure in Debug builds.  The use of
+cast<> implies that the operation cannot fail, and should not be used
+as the condition of a conditional statement.
+
+.. code-block:: c++
+
+  // Finds cases like these:
+  if (auto x = cast(y)) <...>
+  if (cast(y)) <...>
+
+  // But not cases like these:
+  if (auto f = cast(y)->foo()) <...>
+  if (cast(y)->foo()) <...>
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -175,6 +175,7 @@
hicpp-use-nullptr (redirects to modernize-use-nullptr) 
hicpp-use-override (redirects to modernize-use-override) 
hicpp-vararg (redirects to cppcoreguidelines-pro-type-vararg) 
+   llvm-avoid-cast-in-conditional
llvm-header-guard
llvm-include-order
llvm-namespace-comment
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -130,6 +130,12 @@
   ` now supports `OverrideSpelling`
   and `FinalSpelling` options.
 
+- New :doc:`llvm-avoid-cast-in-conditional
+  ` check.
+
+  Finds cases of `cast<>` used as condition in conditional statements
+  which will assert on failure in Debug builds.
+
 - New :doc:`openmp-exception-escape
   ` check.
 
Index: clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
===
--- clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
+++ clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
@@ -14,11 +14,13 @@
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/StringRef.h"
 
+using llvm::SmallSet;
+
 namespace clang {
 namespace tidy {
 namespace utils {
 
-typedef llvm::SmallSet HeaderFileExtensionsSet;
+using HeaderFileExtensionsSet = SmallSet;
 
 /// \brief Checks whether expansion location of \p Loc is in header file.
 bool isExpansionLocInHeaderFile(
Index: clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
===
--- clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
+++ clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
@@ -10,6 +10,7 @@
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "../readability/NamespaceCommentCheck.h"
+#include "AvoidCastInConditionalCheck.h"
 #include "HeaderGuardCheck.h"
 #include "IncludeOrderCheck.h"
 #include "TwineLocalCheck.h"
@@ -21,6 +22,8 @@
 class LLVMModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories ) override {
+CheckFactories.registerCheck(
+"llvm-avoid-cast-in-conditional");
 CheckFactories.registerCheck("llvm-header-guard");
 CheckFactories.registerCheck("llvm-include-order");
 CheckFactories.registerCheck(

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h:23
 
-typedef llvm::SmallSet HeaderFileExtensionsSet;
+typedef SmallSet HeaderFileExtensionsSet;
 

hintonda wrote:
> Eugene.Zelenko wrote:
> > Please use using. See modernize-use-using.
> Normally I'd agree, but since the llvm checkers live in the llvm namespace, 
> including any llvm checker prior to this header will prevent clang from 
> finding `llvm::SmallSet`.  As you can see, the other llvm classes don't need 
> the `llvm::` prefix, but that's because `clang/include/clang/Basic/LLVM.h` 
> already has a bunch of `using` statements.
> 
> So, this code won't compile without this change, or the addition of `using 
> llvm::SmallSet;` to `clang/include/clang/Basic/LLVM.h`.  Should I make that 
> change first?  Then just remove the `llvm::` prefix?
Oh, sorry, didn't read closely enough.  I can get rid of the typedef.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h:23
 
-typedef llvm::SmallSet HeaderFileExtensionsSet;
+typedef SmallSet HeaderFileExtensionsSet;
 

Eugene.Zelenko wrote:
> Please use using. See modernize-use-using.
Normally I'd agree, but since the llvm checkers live in the llvm namespace, 
including any llvm checker prior to this header will prevent clang from finding 
`llvm::SmallSet`.  As you can see, the other llvm classes don't need the 
`llvm::` prefix, but that's because `clang/include/clang/Basic/LLVM.h` already 
has a bunch of `using` statements.

So, this code won't compile without this change, or the addition of `using 
llvm::SmallSet;` to `clang/include/clang/Basic/LLVM.h`.  Should I make that 
change first?  Then just remove the `llvm::` prefix?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h:23
 
-typedef llvm::SmallSet HeaderFileExtensionsSet;
+typedef SmallSet HeaderFileExtensionsSet;
 

Please use using. See modernize-use-using.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:136
+
+  Finds cases of cast<> used as condition in conditional statements which will
+  assert on failure in Debug builds.

Please enclose cast<> in ``.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst:6
+
+Finds cases of cast<> used as the condition of a conditional
+statements which will assert on failure in Debug builds.  The use of

Please enclose cast<> in ``. Same below.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst:7
+Finds cases of cast<> used as the condition of a conditional
+statements which will assert on failure in Debug builds.  The use of
+cast<> implies that the operation cannot fail, and should not be used

Please fix double space.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst:9
+cast<> implies that the operation cannot fail, and should not be used
+as the condition of a conditional statement..
+

Please fix double dot.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59802/new/

https://reviews.llvm.org/D59802



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

hintonda created this revision.
hintonda added reviewers: alexfh, rjmccall.
Herald added subscribers: xazax.hun, mgorny.
Herald added a project: clang.

Finds cases of cast<> used as the condition of a conditional
statements which will assert on failure in Debug builds.  The use of
cast<> implies that the operation cannot fail, and should not be used
as the condition of a conditional statement..

.. code-block:: c++

  // Finds cases like these:
  if (auto x = cast(y)) <...>
  if (cast(y)) <...>
  
  // But not cases like these:
  if (auto f = cast(y)->foo()) <...>
  if (cast(y)->foo()) <...>


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D59802

Files:
  clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp
  clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.h
  clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
  clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
  clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp

Index: clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
@@ -0,0 +1,25 @@
+// RUN: %check_clang_tidy %s llvm-avoid-cast-in-conditional %t
+
+struct X;
+struct Y;
+struct Z {
+  int foo();
+};
+
+template 
+X* cast(Y*);
+
+bool foo(Y* y) {
+  if (auto x = cast(y))
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: {{.*llvm-avoid-cast-in-conditional}}
+return true;
+  if (cast(y))
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning:  {{.*llvm-avoid-cast-in-conditional}}
+return true;
+
+  // Does not trigger warning.
+  if (cast(y)->foo())
+return true;
+  return false;
+}
+
Index: clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
@@ -0,0 +1,19 @@
+.. title:: clang-tidy - llvm-avoid-cast-in-conditional
+
+llvm-avoid-cast-in-conditional
+==
+
+Finds cases of cast<> used as the condition of a conditional
+statements which will assert on failure in Debug builds.  The use of
+cast<> implies that the operation cannot fail, and should not be used
+as the condition of a conditional statement..
+
+.. code-block:: c++
+
+  // Finds cases like these:
+  if (auto x = cast(y)) <...>
+  if (cast(y)) <...>
+
+  // But not cases like these:
+  if (auto f = cast(y)->foo()) <...>
+  if (cast(y)->foo()) <...>
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -175,6 +175,7 @@
hicpp-use-nullptr (redirects to modernize-use-nullptr) 
hicpp-use-override (redirects to modernize-use-override) 
hicpp-vararg (redirects to cppcoreguidelines-pro-type-vararg) 
+   llvm-avoid-cast-in-conditional
llvm-header-guard
llvm-include-order
llvm-namespace-comment
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -130,6 +130,12 @@
   ` now supports `OverrideSpelling`
   and `FinalSpelling` options.
 
+- New :doc:`llvm-avoid-cast-in-conditional
+  ` check.
+
+  Finds cases of cast<> used as condition in conditional statements which will
+  assert on failure in Debug builds.
+
 - New :doc:`openmp-exception-escape
   ` check.
 
Index: clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
===
--- clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
+++ clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
@@ -14,11 +14,13 @@
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/StringRef.h"
 
+using llvm::SmallSet;
+
 namespace clang {
 namespace tidy {
 namespace utils {
 
-typedef llvm::SmallSet HeaderFileExtensionsSet;
+typedef SmallSet HeaderFileExtensionsSet;
 
 /// \brief Checks whether expansion location of \p Loc is in header file.
 bool isExpansionLocInHeaderFile(
Index: clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
===
--- clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
+++ clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
@@ -10,6 +10,7 @@
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "../readability/NamespaceCommentCheck.h"
+#include "AvoidCastInConditionalCheck.h"
 #include