[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman closed this revision.
aaron.ballman added a comment.

Thank you for the patch, I've committed on your behalf in 
ec3d8e61b527c6312f77a4dab095ffc34e954927 



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

https://reviews.llvm.org/D71846



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

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

Yes please, I'm still very new to the llvm contributing system so I would have 
no idea what to do next.


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

https://reviews.llvm.org/D71846



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

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

Still LG -- do you need someone to land this on your behalf?


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

https://reviews.llvm.org/D71846



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

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 235791.
njames93 added a comment.

small reformat


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

https://reviews.llvm.org/D71846

Files:
  clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
  clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h
  
clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return-no-warn.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp
  clang/docs/LibASTMatchersReference.html
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/lib/ASTMatchers/Dynamic/Registry.cpp
  clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -1071,6 +1071,35 @@
  LanguageMode::Cxx17OrLater));
 }
 
+TEST(hasInitStatement, MatchesSelectionInitializers) {
+  EXPECT_TRUE(matches("void baz() { if (int i = 1; i > 0) {} }",
+  ifStmt(hasInitStatement(anything())),
+  LanguageMode::Cxx17OrLater));
+  EXPECT_TRUE(notMatches("void baz() { if (int i = 1) {} }",
+ ifStmt(hasInitStatement(anything();
+  EXPECT_TRUE(notMatches("void baz() { if (1 > 0) {} }",
+ ifStmt(hasInitStatement(anything();
+  EXPECT_TRUE(matches(
+  "void baz(int i) { switch (int j = i; j) { default: break; } }",
+  switchStmt(hasInitStatement(anything())), LanguageMode::Cxx17OrLater));
+  EXPECT_TRUE(notMatches("void baz(int i) { switch (i) { default: break; } }",
+ switchStmt(hasInitStatement(anything();
+}
+
+TEST(hasInitStatement, MatchesRangeForInitializers) {
+  EXPECT_TRUE(matches("void baz() {"
+  "int items[] = {};"
+  "for (auto  = items; auto  : arr) {}"
+  "}",
+  cxxForRangeStmt(hasInitStatement(anything())),
+  LanguageMode::Cxx2aOrLater));
+  EXPECT_TRUE(notMatches("void baz() {"
+ "int items[] = {};"
+ "for (auto  : items) {}"
+ "}",
+ cxxForRangeStmt(hasInitStatement(anything();
+}
+
 TEST(TemplateArgumentCountIs, Matches) {
   EXPECT_TRUE(
 matches("template struct C {}; C c;",
Index: clang/lib/ASTMatchers/Dynamic/Registry.cpp
===
--- clang/lib/ASTMatchers/Dynamic/Registry.cpp
+++ clang/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -279,6 +279,7 @@
   REGISTER_MATCHER(hasIndex);
   REGISTER_MATCHER(hasInit);
   REGISTER_MATCHER(hasInitializer);
+  REGISTER_MATCHER(hasInitStatement);
   REGISTER_MATCHER(hasKeywordSelector);
   REGISTER_MATCHER(hasLHS);
   REGISTER_MATCHER(hasLocalQualifiers);
Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -4297,6 +4297,35 @@
   return Node.isConstexpr();
 }
 
+/// Matches selection statements with initializer.
+///
+/// Given:
+/// \code
+///  void foo() {
+///if (int i = foobar(); i > 0) {}
+///switch (int i = foobar(); i) {}
+///for (auto& a = get_range(); auto& x : a) {}
+///  }
+///  void bar() {
+///if (foobar() > 0) {}
+///switch (foobar()) {}
+///for (auto& x : get_range()) {}
+///  }
+/// \endcode
+/// ifStmt(hasInitStatement(anything()))
+///   matches the if statement in foo but not in bar.
+/// switchStmt(hasInitStatement(anything()))
+///   matches the switch statement in foo but not in bar.
+/// cxxForRangeStmt(hasInitStatement(anything()))
+///   matches the range for statement in foo but not in bar.
+AST_POLYMORPHIC_MATCHER_P(hasInitStatement,
+  AST_POLYMORPHIC_SUPPORTED_TYPES(IfStmt, SwitchStmt,
+  CXXForRangeStmt),
+  internal::Matcher, InnerMatcher) {
+  const Stmt *Init = Node.getInit();
+  return Init != nullptr && InnerMatcher.matches(*Init, Finder, Builder);
+}
+
 /// Matches the condition expression of an if statement, for loop,
 /// switch statement or conditional operator.
 ///
Index: clang/docs/LibASTMatchersReference.html
===
--- clang/docs/LibASTMatchersReference.html
+++ clang/docs/LibASTMatchersReference.html
@@ -3009,6 +3009,23 @@
 
 
 
+Matcherhttps://clang.llvm.org/doxygen/classclang_1_1Expr.html;>ExprnullPointerConstant
+Matches expressions that resolve to a null pointer constant, such as
+GNU's __null, C++11's nullptr, or C's NULL macro.
+
+Given:
+  void *v1 = NULL;
+  void *v2 = nullptr;
+  

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Nathan James via Phabricator via cfe-commits]

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

In D71846#1800503 , @aaron.ballman 
wrote:

> The new option LGTM but one of the tests can be updated to have a less 
> complex RUN line.


that was just put in when i was initially testing the behavoiur, removed it now


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

https://reviews.llvm.org/D71846



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

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 235788.
njames93 added a comment.

removed the unnecessary explicit setting of WarnOnUnfixable value for test case


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

https://reviews.llvm.org/D71846

Files:
  clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
  clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h
  
clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return-no-warn.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp
  clang/docs/LibASTMatchersReference.html
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/lib/ASTMatchers/Dynamic/Registry.cpp
  clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -1071,6 +1071,35 @@
  LanguageMode::Cxx17OrLater));
 }
 
+TEST(hasInitStatement, MatchesSelectionInitializers) {
+  EXPECT_TRUE(matches("void baz() { if (int i = 1; i > 0) {} }",
+  ifStmt(hasInitStatement(anything())),
+  LanguageMode::Cxx17OrLater));
+  EXPECT_TRUE(notMatches("void baz() { if (int i = 1) {} }",
+ ifStmt(hasInitStatement(anything();
+  EXPECT_TRUE(notMatches("void baz() { if (1 > 0) {} }",
+ ifStmt(hasInitStatement(anything();
+  EXPECT_TRUE(matches(
+  "void baz(int i) { switch (int j = i; j) { default: break; } }",
+  switchStmt(hasInitStatement(anything())), LanguageMode::Cxx17OrLater));
+  EXPECT_TRUE(notMatches("void baz(int i) { switch (i) { default: break; } }",
+ switchStmt(hasInitStatement(anything();
+}
+
+TEST(hasInitStatement, MatchesRangeForInitializers) {
+  EXPECT_TRUE(matches("void baz() {"
+  "int items[] = {};"
+  "for (auto  = items; auto  : arr) {}"
+  "}",
+  cxxForRangeStmt(hasInitStatement(anything())),
+  LanguageMode::Cxx2aOrLater));
+  EXPECT_TRUE(notMatches("void baz() {"
+ "int items[] = {};"
+ "for (auto  : items) {}"
+ "}",
+ cxxForRangeStmt(hasInitStatement(anything();
+}
+
 TEST(TemplateArgumentCountIs, Matches) {
   EXPECT_TRUE(
 matches("template struct C {}; C c;",
Index: clang/lib/ASTMatchers/Dynamic/Registry.cpp
===
--- clang/lib/ASTMatchers/Dynamic/Registry.cpp
+++ clang/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -279,6 +279,7 @@
   REGISTER_MATCHER(hasIndex);
   REGISTER_MATCHER(hasInit);
   REGISTER_MATCHER(hasInitializer);
+  REGISTER_MATCHER(hasInitStatement);
   REGISTER_MATCHER(hasKeywordSelector);
   REGISTER_MATCHER(hasLHS);
   REGISTER_MATCHER(hasLocalQualifiers);
Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -4297,6 +4297,35 @@
   return Node.isConstexpr();
 }
 
+/// Matches selection statements with initializer.
+///
+/// Given:
+/// \code
+///  void foo() {
+///if (int i = foobar(); i > 0) {}
+///switch (int i = foobar(); i) {}
+///for (auto& a = get_range(); auto& x : a) {}
+///  }
+///  void bar() {
+///if (foobar() > 0) {}
+///switch (foobar()) {}
+///for (auto& x : get_range()) {}
+///  }
+/// \endcode
+/// ifStmt(hasInitStatement(anything()))
+///   matches the if statement in foo but not in bar.
+/// switchStmt(hasInitStatement(anything()))
+///   matches the switch statement in foo but not in bar.
+/// cxxForRangeStmt(hasInitStatement(anything()))
+///   matches the range for statement in foo but not in bar.
+AST_POLYMORPHIC_MATCHER_P(hasInitStatement,
+  AST_POLYMORPHIC_SUPPORTED_TYPES(IfStmt, SwitchStmt,
+  CXXForRangeStmt),
+  internal::Matcher, InnerMatcher) {
+  const Stmt *Init = Node.getInit();
+  return Init != nullptr && InnerMatcher.matches(*Init, Finder, Builder);
+}
+
 /// Matches the condition expression of an if statement, for loop,
 /// switch statement or conditional operator.
 ///
Index: clang/docs/LibASTMatchersReference.html
===
--- clang/docs/LibASTMatchersReference.html
+++ clang/docs/LibASTMatchersReference.html
@@ -3009,6 +3009,23 @@
 
 
 
+Matcherhttps://clang.llvm.org/doxygen/classclang_1_1Expr.html;>ExprnullPointerConstant
+Matches expressions that resolve to a null pointer constant, such as
+GNU's __null, C++11's nullptr, or C's NULL 

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

The new option LGTM but one of the tests can be updated to have a less complex 
RUN line.




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp:2-3
+// RUN: %check_clang_tidy %s readability-else-after-return %t -- \
+// RUN: -config='{CheckOptions: [ \
+// RUN: {key: readability-else-after-return.WarnOnUnfixable, value: 
1}, \
+// RUN: ]}' -- -fexceptions -std=c++17

You don't need to explicitly set it to `1` here.


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

https://reviews.llvm.org/D71846



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

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 235775.
njames93 added a comment.

Added option to disable warning when an automatic fix can't be applied due to 
scope restrictions of variables, default option is to show all warnings


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

https://reviews.llvm.org/D71846

Files:
  clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
  clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h
  
clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return-no-warn.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp
  clang/docs/LibASTMatchersReference.html
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/lib/ASTMatchers/Dynamic/Registry.cpp
  clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -1071,6 +1071,35 @@
  LanguageMode::Cxx17OrLater));
 }
 
+TEST(hasInitStatement, MatchesSelectionInitializers) {
+  EXPECT_TRUE(matches("void baz() { if (int i = 1; i > 0) {} }",
+  ifStmt(hasInitStatement(anything())),
+  LanguageMode::Cxx17OrLater));
+  EXPECT_TRUE(notMatches("void baz() { if (int i = 1) {} }",
+ ifStmt(hasInitStatement(anything();
+  EXPECT_TRUE(notMatches("void baz() { if (1 > 0) {} }",
+ ifStmt(hasInitStatement(anything();
+  EXPECT_TRUE(matches(
+  "void baz(int i) { switch (int j = i; j) { default: break; } }",
+  switchStmt(hasInitStatement(anything())), LanguageMode::Cxx17OrLater));
+  EXPECT_TRUE(notMatches("void baz(int i) { switch (i) { default: break; } }",
+ switchStmt(hasInitStatement(anything();
+}
+
+TEST(hasInitStatement, MatchesRangeForInitializers) {
+  EXPECT_TRUE(matches("void baz() {"
+  "int items[] = {};"
+  "for (auto  = items; auto  : arr) {}"
+  "}",
+  cxxForRangeStmt(hasInitStatement(anything())),
+  LanguageMode::Cxx2aOrLater));
+  EXPECT_TRUE(notMatches("void baz() {"
+ "int items[] = {};"
+ "for (auto  : items) {}"
+ "}",
+ cxxForRangeStmt(hasInitStatement(anything();
+}
+
 TEST(TemplateArgumentCountIs, Matches) {
   EXPECT_TRUE(
 matches("template struct C {}; C c;",
Index: clang/lib/ASTMatchers/Dynamic/Registry.cpp
===
--- clang/lib/ASTMatchers/Dynamic/Registry.cpp
+++ clang/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -279,6 +279,7 @@
   REGISTER_MATCHER(hasIndex);
   REGISTER_MATCHER(hasInit);
   REGISTER_MATCHER(hasInitializer);
+  REGISTER_MATCHER(hasInitStatement);
   REGISTER_MATCHER(hasKeywordSelector);
   REGISTER_MATCHER(hasLHS);
   REGISTER_MATCHER(hasLocalQualifiers);
Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -4297,6 +4297,35 @@
   return Node.isConstexpr();
 }
 
+/// Matches selection statements with initializer.
+///
+/// Given:
+/// \code
+///  void foo() {
+///if (int i = foobar(); i > 0) {}
+///switch (int i = foobar(); i) {}
+///for (auto& a = get_range(); auto& x : a) {}
+///  }
+///  void bar() {
+///if (foobar() > 0) {}
+///switch (foobar()) {}
+///for (auto& x : get_range()) {}
+///  }
+/// \endcode
+/// ifStmt(hasInitStatement(anything()))
+///   matches the if statement in foo but not in bar.
+/// switchStmt(hasInitStatement(anything()))
+///   matches the switch statement in foo but not in bar.
+/// cxxForRangeStmt(hasInitStatement(anything()))
+///   matches the range for statement in foo but not in bar.
+AST_POLYMORPHIC_MATCHER_P(hasInitStatement,
+  AST_POLYMORPHIC_SUPPORTED_TYPES(IfStmt, SwitchStmt,
+  CXXForRangeStmt),
+  internal::Matcher, InnerMatcher) {
+  const Stmt *Init = Node.getInit();
+  return Init != nullptr && InnerMatcher.matches(*Init, Finder, Builder);
+}
+
 /// Matches the condition expression of an if statement, for loop,
 /// switch statement or conditional operator.
 ///
Index: clang/docs/LibASTMatchersReference.html
===
--- clang/docs/LibASTMatchersReference.html
+++ clang/docs/LibASTMatchersReference.html
@@ -3009,6 +3009,23 @@
 
 
 
+Matcherhttps://clang.llvm.org/doxygen/classclang_1_1Expr.html;>ExprnullPointerConstant
+Matches expressions that resolve to a null 

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

In D71846#1800481 , @aaron.ballman 
wrote:

> In D71846#1800480 , @njames93 wrote:
>
> > Definitely default to diagnosing everything, that's how my current 
> > implementation looks right now.
>
>
> SGTM
>
> > Also is there a way to do options as bool because I don't particularly like 
> > putting 1 for true in the config file
>
> Not to my knowledge.


I just did a quick grep and found that all checks that take bools for config 
options use a 1 for true, 0 for false, so I guess best stick to convention 
rather than hack at OptionsView to get and set bools


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

https://reviews.llvm.org/D71846



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

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D71846#1800480 , @njames93 wrote:

> In D71846#1800411 , @aaron.ballman 
> wrote:
>
> > In D71846#1800401 , @njames93 
> > wrote:
> >
> > > In D71846#1800381 , 
> > > @aaron.ballman wrote:
> > >
> > > > In D71846#1800344 , @njames93 
> > > > wrote:
> > > >
> > > > > I'm in two minds about issuing a warning when scope restrictions 
> > > > > prevent a fix. Do you think creating an option to enable or disable 
> > > > > emitting warnings for cases where the scope prevents a fix would be a 
> > > > > good idea?
> > > >
> > > >
> > > > It's not uncommon for fixits to only be generated under specific 
> > > > circumstances, so I'm wondering what your concern is with warning when 
> > > > we can't provide a fixit? The cases I am thinking about all seem 
> > > > reasonable to diagnose (are true positives) without fixing, but maybe 
> > > > you have different circumstances in mind.
> > >
> > >
> > > Right now an issue is raised for every else after return flag, but not 
> > > all else after return flags can be fixed due to declaration statements 
> > > and scope issues. My suggestion is that you can choose to warn about 
> > > those cases or not. For example a developer may want else after return 
> > > for when they need to limit the scope and getting a warning for it may be 
> > > undesirable.
> >
> >
> > Okay, I can see the value in having an option for that -- especially given 
> > that silencing the diagnostic would add `// NOLINT` noise to the code. Do 
> > you think the option should default to diagnosing all cases, even ones 
> > without a fixit?
>
>
> Definitely default to diagnosing everything, that's how my current 
> implementation looks right now.


SGTM

> Also is there a way to do options as bool because I don't particularly like 
> putting 1 for true in the config file

Not to my knowledge.


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

https://reviews.llvm.org/D71846



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

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

In D71846#1800411 , @aaron.ballman 
wrote:

> In D71846#1800401 , @njames93 wrote:
>
> > In D71846#1800381 , @aaron.ballman 
> > wrote:
> >
> > > In D71846#1800344 , @njames93 
> > > wrote:
> > >
> > > > I'm in two minds about issuing a warning when scope restrictions 
> > > > prevent a fix. Do you think creating an option to enable or disable 
> > > > emitting warnings for cases where the scope prevents a fix would be a 
> > > > good idea?
> > >
> > >
> > > It's not uncommon for fixits to only be generated under specific 
> > > circumstances, so I'm wondering what your concern is with warning when we 
> > > can't provide a fixit? The cases I am thinking about all seem reasonable 
> > > to diagnose (are true positives) without fixing, but maybe you have 
> > > different circumstances in mind.
> >
> >
> > Right now an issue is raised for every else after return flag, but not all 
> > else after return flags can be fixed due to declaration statements and 
> > scope issues. My suggestion is that you can choose to warn about those 
> > cases or not. For example a developer may want else after return for when 
> > they need to limit the scope and getting a warning for it may be 
> > undesirable.
>
>
> Okay, I can see the value in having an option for that -- especially given 
> that silencing the diagnostic would add `// NOLINT` noise to the code. Do you 
> think the option should default to diagnosing all cases, even ones without a 
> fixit?


Definitely default to diagnosing everything, that's how my current 
implementation looks right now. Also is there a way to do options as bool 
because I don't particularly like putting 1 for true in the config file


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

https://reviews.llvm.org/D71846



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

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D71846#1800401 , @njames93 wrote:

> In D71846#1800381 , @aaron.ballman 
> wrote:
>
> > In D71846#1800344 , @njames93 
> > wrote:
> >
> > > I'm in two minds about issuing a warning when scope restrictions prevent 
> > > a fix. Do you think creating an option to enable or disable emitting 
> > > warnings for cases where the scope prevents a fix would be a good idea?
> >
> >
> > It's not uncommon for fixits to only be generated under specific 
> > circumstances, so I'm wondering what your concern is with warning when we 
> > can't provide a fixit? The cases I am thinking about all seem reasonable to 
> > diagnose (are true positives) without fixing, but maybe you have different 
> > circumstances in mind.
>
>
> Right now an issue is raised for every else after return flag, but not all 
> else after return flags can be fixed due to declaration statements and scope 
> issues. My suggestion is that you can choose to warn about those cases or 
> not. For example a developer may want else after return for when they need to 
> limit the scope and getting a warning for it may be undesirable.


Okay, I can see the value in having an option for that -- especially given that 
silencing the diagnostic would add `// NOLINT` noise to the code. Do you think 
the option should default to diagnosing all cases, even ones without a fixit?


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

https://reviews.llvm.org/D71846



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

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

In D71846#1800381 , @aaron.ballman 
wrote:

> In D71846#1800344 , @njames93 wrote:
>
> > I'm in two minds about issuing a warning when scope restrictions prevent a 
> > fix. Do you think creating an option to enable or disable emitting warnings 
> > for cases where the scope prevents a fix would be a good idea?
>
>
> It's not uncommon for fixits to only be generated under specific 
> circumstances, so I'm wondering what your concern is with warning when we 
> can't provide a fixit? The cases I am thinking about all seem reasonable to 
> diagnose (are true positives) without fixing, but maybe you have different 
> circumstances in mind.


Right now an issue is raised for every else after return flag, but not all else 
after return flags can be fixed due to declaration statements and scope issues. 
My suggestion is that you can choose to warn about those cases or not. For 
example a developer may want else after return for when they need to limit the 
scope and getting a warning for it may be undesirable.


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

https://reviews.llvm.org/D71846



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

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D71846#1800344 , @njames93 wrote:

> I'm in two minds about issuing a warning when scope restrictions prevent a 
> fix. Do you think creating an option to enable or disable emitting warnings 
> for cases where the scope prevents a fix would be a good idea?


It's not uncommon for fixits to only be generated under specific circumstances, 
so I'm wondering what your concern is with warning when we can't provide a 
fixit? The cases I am thinking about all seem reasonable to diagnose (are true 
positives) without fixing, but maybe you have different circumstances in mind.


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

https://reviews.llvm.org/D71846



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

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

I'm in two minds about issuing a warning when scope restrictions prevent a fix. 
Do you think creating an option to enable or disable emitting warnings for 
cases where the scope prevents a fix would be a good idea?


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

https://reviews.llvm.org/D71846



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

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 235752.
njames93 added a comment.

hopefully adhered to all conventions :)


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

https://reviews.llvm.org/D71846

Files:
  clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp
  clang/docs/LibASTMatchersReference.html
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/lib/ASTMatchers/Dynamic/Registry.cpp
  clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -1071,6 +1071,35 @@
  LanguageMode::Cxx17OrLater));
 }
 
+TEST(hasInitStatement, MatchesSelectionInitializers) {
+  EXPECT_TRUE(matches("void baz() { if (int i = 1; i > 0) {} }",
+  ifStmt(hasInitStatement(anything())),
+  LanguageMode::Cxx17OrLater));
+  EXPECT_TRUE(notMatches("void baz() { if (int i = 1) {} }",
+ ifStmt(hasInitStatement(anything();
+  EXPECT_TRUE(notMatches("void baz() { if (1 > 0) {} }",
+ ifStmt(hasInitStatement(anything();
+  EXPECT_TRUE(matches(
+  "void baz(int i) { switch (int j = i; j) { default: break; } }",
+  switchStmt(hasInitStatement(anything())), LanguageMode::Cxx17OrLater));
+  EXPECT_TRUE(notMatches("void baz(int i) { switch (i) { default: break; } }",
+ switchStmt(hasInitStatement(anything();
+}
+
+TEST(hasInitStatement, MatchesRangeForInitializers) {
+  EXPECT_TRUE(matches("void baz() {"
+  "int items[] = {};"
+  "for (auto  = items; auto  : arr) {}"
+  "}",
+  cxxForRangeStmt(hasInitStatement(anything())),
+  LanguageMode::Cxx2aOrLater));
+  EXPECT_TRUE(notMatches("void baz() {"
+ "int items[] = {};"
+ "for (auto  : items) {}"
+ "}",
+ cxxForRangeStmt(hasInitStatement(anything();
+}
+
 TEST(TemplateArgumentCountIs, Matches) {
   EXPECT_TRUE(
 matches("template struct C {}; C c;",
Index: clang/lib/ASTMatchers/Dynamic/Registry.cpp
===
--- clang/lib/ASTMatchers/Dynamic/Registry.cpp
+++ clang/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -279,6 +279,7 @@
   REGISTER_MATCHER(hasIndex);
   REGISTER_MATCHER(hasInit);
   REGISTER_MATCHER(hasInitializer);
+  REGISTER_MATCHER(hasInitStatement);
   REGISTER_MATCHER(hasKeywordSelector);
   REGISTER_MATCHER(hasLHS);
   REGISTER_MATCHER(hasLocalQualifiers);
Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -4297,6 +4297,35 @@
   return Node.isConstexpr();
 }
 
+/// Matches selection statements with initializer.
+///
+/// Given:
+/// \code
+///  void foo() {
+///if (int i = foobar(); i > 0) {}
+///switch (int i = foobar(); i) {}
+///for (auto& a = get_range(); auto& x : a) {}
+///  }
+///  void bar() {
+///if (foobar() > 0) {}
+///switch (foobar()) {}
+///for (auto& x : get_range()) {}
+///  }
+/// \endcode
+/// ifStmt(hasInitStatement(anything()))
+///   matches the if statement in foo but not in bar.
+/// switchStmt(hasInitStatement(anything()))
+///   matches the switch statement in foo but not in bar.
+/// cxxForRangeStmt(hasInitStatement(anything()))
+///   matches the range for statement in foo but not in bar.
+AST_POLYMORPHIC_MATCHER_P(hasInitStatement,
+  AST_POLYMORPHIC_SUPPORTED_TYPES(IfStmt, SwitchStmt,
+  CXXForRangeStmt),
+  internal::Matcher, InnerMatcher) {
+  const Stmt *Init = Node.getInit();
+  return Init != nullptr && InnerMatcher.matches(*Init, Finder, Builder);
+}
+
 /// Matches the condition expression of an if statement, for loop,
 /// switch statement or conditional operator.
 ///
Index: clang/docs/LibASTMatchersReference.html
===
--- clang/docs/LibASTMatchersReference.html
+++ clang/docs/LibASTMatchersReference.html
@@ -3009,6 +3009,23 @@
 
 
 
+Matcherhttps://clang.llvm.org/doxygen/classclang_1_1Expr.html;>ExprnullPointerConstant
+Matches expressions that resolve to a null pointer constant, such as
+GNU's __null, C++11's nullptr, or C's NULL macro.
+
+Given:
+  void *v1 = NULL;
+  void *v2 = nullptr;
+  void *v3 = __null; // GNU extension
+  char *cp = (char *)0;
+  int *ip = 0;
+  int i = 0;
+expr(nullPointerConstant())
+  matches 

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D71846#1798115 , @njames93 wrote:

> Sorry I didn't make it obvious with the test cases. The fix will never extend 
> the lifetime of variables either in the condition or declared in the else 
> block. It will only apply if the if is the last statement in its scope. 
> Though I should check back through the scope for any declarations that have 
> the same identifier as those in the if statement which would cause an error 
> if they were brought out of the if scope


Oh, thank you for the clarification, I hadn't picked up on that.




Comment at: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp:21
 
+static const char ReturnStr[] = "return";
+static const char ContinueStr[] = "continue";

Hmm, I just realized we missed `co_return` for this check. That's a separate 
patch, though, so don't feel the need to change anything about this here.



Comment at: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp:45
+const DeclRefExpr *findUsage(const Stmt *Node, int64_t DeclIdentifier) {
+  if (const auto *DeclRef = dyn_cast_or_null(Node)) {
+if (DeclRef->getDecl()->getID() == DeclIdentifier) {

The use of `dyn_cast_or_null` is suspicious in these methods -- if `Node` is 
null, then we'll get into the `else` clause and promptly dereference a null 
pointer. My guess is that these should be `dyn_cast` calls instead, but if 
`Node` can truly be null, that case should be handled.



Comment at: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp:50
+  } else {
+for (const auto  : Node->children()) {
+  if (auto Result = findUsage(ChildNode, DeclIdentifier)) {

`const auto *`, no? (Same below.)



Comment at: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp:51
+for (const auto  : Node->children()) {
+  if (auto Result = findUsage(ChildNode, DeclIdentifier)) {
+return Result;

`const auto *` for sure. (Same below.)



Comment at: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp:77
+const DeclRefExpr *checkInitDeclUsageInElse(const IfStmt *If) {
+  auto InitDeclStmt = dyn_cast_or_null(If->getInit());
+  if (!InitDeclStmt)

`const auto *` (this comment applies throughout the patch -- we don't want to 
deduce qualifiers or pointer/references, so they should be spelled out 
explicitly.)



Comment at: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp:86
+  llvm::SmallVector DeclIdentifiers;
+  for (const auto  : InitDeclStmt->decls()) {
+assert(isa(Decl) && "Init Decls must be a VarDecl");

`const auto *`, no?



Comment at: 
clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp:123
+Diag << tooling::fixit::createRemoval(ElseLoc);
+const auto LBrace = CS->getLBracLoc();
+const auto RBrace = CS->getRBracLoc();

Please don't use `auto` here as the type is not spelled out in the 
initialization. Also, drop the top-level `const` qualifier. Same elsewhere.



Comment at: 
clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp:161
+  if (!IsLastInScope && containsDeclInScope(Else)) {
+// warn, but don't attempt an autofix
+diag(ElseLoc, WarningMessage) << ControlFlowInterruptor;

warn -> Warn
and should have a full stop at the end of the comment. The same applies to the 
other instances of this comment.


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

https://reviews.llvm.org/D71846



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

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 235661.
njames93 added a comment.

Fixed a few of the test cases failing


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

https://reviews.llvm.org/D71846

Files:
  clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp
  clang/docs/LibASTMatchersReference.html
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/lib/ASTMatchers/Dynamic/Registry.cpp
  clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -1071,6 +1071,35 @@
  LanguageMode::Cxx17OrLater));
 }
 
+TEST(hasInitStatement, MatchesSelectionInitializers) {
+  EXPECT_TRUE(matches("void baz() { if (int i = 1; i > 0) {} }",
+  ifStmt(hasInitStatement(anything())),
+  LanguageMode::Cxx17OrLater));
+  EXPECT_TRUE(notMatches("void baz() { if (int i = 1) {} }",
+ ifStmt(hasInitStatement(anything();
+  EXPECT_TRUE(notMatches("void baz() { if (1 > 0) {} }",
+ ifStmt(hasInitStatement(anything();
+  EXPECT_TRUE(matches(
+  "void baz(int i) { switch (int j = i; j) { default: break; } }",
+  switchStmt(hasInitStatement(anything())), LanguageMode::Cxx17OrLater));
+  EXPECT_TRUE(notMatches("void baz(int i) { switch (i) { default: break; } }",
+ switchStmt(hasInitStatement(anything();
+}
+
+TEST(hasInitStatement, MatchesRangeForInitializers) {
+  EXPECT_TRUE(matches("void baz() {"
+  "int items[] = {};"
+  "for (auto  = items; auto  : arr) {}"
+  "}",
+  cxxForRangeStmt(hasInitStatement(anything())),
+  LanguageMode::Cxx2aOrLater));
+  EXPECT_TRUE(notMatches("void baz() {"
+ "int items[] = {};"
+ "for (auto  : items) {}"
+ "}",
+ cxxForRangeStmt(hasInitStatement(anything();
+}
+
 TEST(TemplateArgumentCountIs, Matches) {
   EXPECT_TRUE(
 matches("template struct C {}; C c;",
Index: clang/lib/ASTMatchers/Dynamic/Registry.cpp
===
--- clang/lib/ASTMatchers/Dynamic/Registry.cpp
+++ clang/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -279,6 +279,7 @@
   REGISTER_MATCHER(hasIndex);
   REGISTER_MATCHER(hasInit);
   REGISTER_MATCHER(hasInitializer);
+  REGISTER_MATCHER(hasInitStatement);
   REGISTER_MATCHER(hasKeywordSelector);
   REGISTER_MATCHER(hasLHS);
   REGISTER_MATCHER(hasLocalQualifiers);
Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -4297,6 +4297,35 @@
   return Node.isConstexpr();
 }
 
+/// Matches selection statements with initializer.
+///
+/// Given:
+/// \code
+///  void foo() {
+///if (int i = foobar(); i > 0) {}
+///switch (int i = foobar(); i) {}
+///for (auto& a = get_range(); auto& x : a) {}
+///  }
+///  void bar() {
+///if (foobar() > 0) {}
+///switch (foobar()) {}
+///for (auto& x : get_range()) {}
+///  }
+/// \endcode
+/// ifStmt(hasInitStatement(anything()))
+///   matches the if statement in foo but not in bar.
+/// switchStmt(hasInitStatement(anything()))
+///   matches the switch statement in foo but not in bar.
+/// cxxForRangeStmt(hasInitStatement(anything()))
+///   matches the range for statement in foo but not in bar.
+AST_POLYMORPHIC_MATCHER_P(hasInitStatement,
+  AST_POLYMORPHIC_SUPPORTED_TYPES(IfStmt, SwitchStmt,
+  CXXForRangeStmt),
+  internal::Matcher, InnerMatcher) {
+  const Stmt *Init = Node.getInit();
+  return Init != nullptr && InnerMatcher.matches(*Init, Finder, Builder);
+}
+
 /// Matches the condition expression of an if statement, for loop,
 /// switch statement or conditional operator.
 ///
Index: clang/docs/LibASTMatchersReference.html
===
--- clang/docs/LibASTMatchersReference.html
+++ clang/docs/LibASTMatchersReference.html
@@ -3009,6 +3009,23 @@
 
 
 
+Matcherhttps://clang.llvm.org/doxygen/classclang_1_1Expr.html;>ExprnullPointerConstant
+Matches expressions that resolve to a null pointer constant, such as
+GNU's __null, C++11's nullptr, or C's NULL macro.
+
+Given:
+  void *v1 = NULL;
+  void *v2 = nullptr;
+  void *v3 = __null; // GNU extension
+  char *cp = (char *)0;
+  int *ip = 0;
+  int i = 0;
+expr(nullPointerConstant())
+  matches 

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 235573.
njames93 added a comment.

Fixed a few more edge cases with test cases


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

https://reviews.llvm.org/D71846

Files:
  clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp
  clang/docs/LibASTMatchersReference.html
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/lib/ASTMatchers/Dynamic/Registry.cpp
  clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -1071,6 +1071,35 @@
  LanguageMode::Cxx17OrLater));
 }
 
+TEST(hasInitStatement, MatchesSelectionInitializers) {
+  EXPECT_TRUE(matches("void baz() { if (int i = 1; i > 0) {} }",
+  ifStmt(hasInitStatement(anything())),
+  LanguageMode::Cxx17OrLater));
+  EXPECT_TRUE(notMatches("void baz() { if (int i = 1) {} }",
+ ifStmt(hasInitStatement(anything();
+  EXPECT_TRUE(notMatches("void baz() { if (1 > 0) {} }",
+ ifStmt(hasInitStatement(anything();
+  EXPECT_TRUE(matches(
+  "void baz(int i) { switch (int j = i; j) { default: break; } }",
+  switchStmt(hasInitStatement(anything())), LanguageMode::Cxx17OrLater));
+  EXPECT_TRUE(notMatches("void baz(int i) { switch (i) { default: break; } }",
+ switchStmt(hasInitStatement(anything();
+}
+
+TEST(hasInitStatement, MatchesRangeForInitializers) {
+  EXPECT_TRUE(matches("void baz() {"
+  "int items[] = {};"
+  "for (auto  = items; auto  : arr) {}"
+  "}",
+  cxxForRangeStmt(hasInitStatement(anything())),
+  LanguageMode::Cxx2aOrLater));
+  EXPECT_TRUE(notMatches("void baz() {"
+ "int items[] = {};"
+ "for (auto  : items) {}"
+ "}",
+ cxxForRangeStmt(hasInitStatement(anything();
+}
+
 TEST(TemplateArgumentCountIs, Matches) {
   EXPECT_TRUE(
 matches("template struct C {}; C c;",
Index: clang/lib/ASTMatchers/Dynamic/Registry.cpp
===
--- clang/lib/ASTMatchers/Dynamic/Registry.cpp
+++ clang/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -279,6 +279,7 @@
   REGISTER_MATCHER(hasIndex);
   REGISTER_MATCHER(hasInit);
   REGISTER_MATCHER(hasInitializer);
+  REGISTER_MATCHER(hasInitStatement);
   REGISTER_MATCHER(hasKeywordSelector);
   REGISTER_MATCHER(hasLHS);
   REGISTER_MATCHER(hasLocalQualifiers);
Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -4297,6 +4297,35 @@
   return Node.isConstexpr();
 }
 
+/// Matches selection statements with initializer.
+///
+/// Given:
+/// \code
+///  void foo() {
+///if (int i = foobar(); i > 0) {}
+///switch (int i = foobar(); i) {}
+///for (auto& a = get_range(); auto& x : a) {}
+///  }
+///  void bar() {
+///if (foobar() > 0) {}
+///switch (foobar()) {}
+///for (auto& x : get_range()) {}
+///  }
+/// \endcode
+/// ifStmt(hasInitStatement(anything()))
+///   matches the if statement in foo but not in bar.
+/// switchStmt(hasInitStatement(anything()))
+///   matches the switch statement in foo but not in bar.
+/// cxxForRangeStmt(hasInitStatement(anything()))
+///   matches the range for statement in foo but not in bar.
+AST_POLYMORPHIC_MATCHER_P(hasInitStatement,
+  AST_POLYMORPHIC_SUPPORTED_TYPES(IfStmt, SwitchStmt,
+  CXXForRangeStmt),
+  internal::Matcher, InnerMatcher) {
+  const Stmt *Init = Node.getInit();
+  return Init != nullptr && InnerMatcher.matches(*Init, Finder, Builder);
+}
+
 /// Matches the condition expression of an if statement, for loop,
 /// switch statement or conditional operator.
 ///
Index: clang/docs/LibASTMatchersReference.html
===
--- clang/docs/LibASTMatchersReference.html
+++ clang/docs/LibASTMatchersReference.html
@@ -3009,6 +3009,23 @@
 
 
 
+Matcherhttps://clang.llvm.org/doxygen/classclang_1_1Expr.html;>ExprnullPointerConstant
+Matches expressions that resolve to a null pointer constant, such as
+GNU's __null, C++11's nullptr, or C's NULL macro.
+
+Given:
+  void *v1 = NULL;
+  void *v2 = nullptr;
+  void *v3 = __null; // GNU extension
+  char *cp = (char *)0;
+  int *ip = 0;
+  int i = 0;
+expr(nullPointerConstant())
+  

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

Adding checks to see if there are any declarations already in the ifs contained 
scope is really starting to bloat the code while trying to cover every edge 
case.


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

https://reviews.llvm.org/D71846



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

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

Sorry I didn't make it obvious with the test cases. The fix will never extend 
the lifetime of variables either in the condition or declared in the else 
block. It will only apply if the if is the last statement in its scope. Though 
I should check back through the scope for any declarations that have the same 
identifier as those in the if statement which would cause an error if they were 
brought out of the if scope


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

https://reviews.llvm.org/D71846



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

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D71846#1796883 , @njames93 wrote:

> So I wasn't happy with the vagueness of the else after return check 
> implementation. Almost finished rewriting the check to properly handle the if 
> statements with condition variables based on scope restrictions and where 
> decls are used etc.
>
>   int *varInitAndCondition() {
> if (int *X = g(); X != nullptr) {
>   return X;
> } else {
>   h();
>   return X;
> }
>   }
>   int *varInitAndConditionUnusedInElseWithDecl() {
> int *Y = g();
> if (int *X = g(); X != nullptr) {
>   return X;
> } else {
>   int *Y = g();
>   h();
> }
> return Y;
>   }
>
>
> transforms to
>
>   int *varInitAndCondition() {
> int *X = g();
> if (X != nullptr) {
>   return X;
> }
> h();
> return X;
>   }
>   int *varInitAndConditionUnusedInElseWithDecl() {
> int *Y = g();
> if (int *X = g(); X != nullptr) {
>   return X;
> } else {
>   int *Y = g();
>   h();
> }
> return Y;
>   }
>
>
> There's a few more test cases but that's the general idea. I'm having a 
> little trouble writing the test cases as I can't run them on my windows 
> machine to verify they report correctly


Hmm, I'm not convinced the new behavior is better in all cases, and I suspect 
this may boil down to user preference (suggesting we may want an option to 
control the behavior). There are competing stylistic decisions to be made for 
`varInitAndCondition()` -- one is the `else` after a `return`, but the other is 
the scope of the variable `X`. The behavior now forces the user to prefer 
removing the `else` after the `return` over hiding the variable `X` in a 
logical scope. For the contrived example provided, this isn't a huge deal 
because the function is small. However, in real-world code with larger function 
bodies, I can see wanting to prefer the name hiding. WDYT?


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

https://reviews.llvm.org/D71846



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

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

Should also point out that clang-format now has a better time reformatting as i 
expanded the replacements range to include the entire else block rather than 
the start and end braces


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

https://reviews.llvm.org/D71846



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

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 235384.
njames93 edited the summary of this revision.
njames93 added a comment.

Think all the test cases are working, but they look like they could do with a 
clean up from someone who may know the clang-tidy-checks system better than 
myself


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

https://reviews.llvm.org/D71846

Files:
  clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp
  clang/docs/LibASTMatchersReference.html
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/lib/ASTMatchers/Dynamic/Registry.cpp
  clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -1071,6 +1071,35 @@
  LanguageMode::Cxx17OrLater));
 }
 
+TEST(hasInitStatement, MatchesSelectionInitializers) {
+  EXPECT_TRUE(matches("void baz() { if (int i = 1; i > 0) {} }",
+  ifStmt(hasInitStatement(anything())),
+  LanguageMode::Cxx17OrLater));
+  EXPECT_TRUE(notMatches("void baz() { if (int i = 1) {} }",
+ ifStmt(hasInitStatement(anything();
+  EXPECT_TRUE(notMatches("void baz() { if (1 > 0) {} }",
+ ifStmt(hasInitStatement(anything();
+  EXPECT_TRUE(matches(
+  "void baz(int i) { switch (int j = i; j) { default: break; } }",
+  switchStmt(hasInitStatement(anything())), LanguageMode::Cxx17OrLater));
+  EXPECT_TRUE(notMatches("void baz(int i) { switch (i) { default: break; } }",
+ switchStmt(hasInitStatement(anything();
+}
+
+TEST(hasInitStatement, MatchesRangeForInitializers) {
+  EXPECT_TRUE(matches("void baz() {"
+  "int items[] = {};"
+  "for (auto  = items; auto  : arr) {}"
+  "}",
+  cxxForRangeStmt(hasInitStatement(anything())),
+  LanguageMode::Cxx2aOrLater));
+  EXPECT_TRUE(notMatches("void baz() {"
+ "int items[] = {};"
+ "for (auto  : items) {}"
+ "}",
+ cxxForRangeStmt(hasInitStatement(anything();
+}
+
 TEST(TemplateArgumentCountIs, Matches) {
   EXPECT_TRUE(
 matches("template struct C {}; C c;",
Index: clang/lib/ASTMatchers/Dynamic/Registry.cpp
===
--- clang/lib/ASTMatchers/Dynamic/Registry.cpp
+++ clang/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -279,6 +279,7 @@
   REGISTER_MATCHER(hasIndex);
   REGISTER_MATCHER(hasInit);
   REGISTER_MATCHER(hasInitializer);
+  REGISTER_MATCHER(hasInitStatement);
   REGISTER_MATCHER(hasKeywordSelector);
   REGISTER_MATCHER(hasLHS);
   REGISTER_MATCHER(hasLocalQualifiers);
Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -4297,6 +4297,35 @@
   return Node.isConstexpr();
 }
 
+/// Matches selection statements with initializer.
+///
+/// Given:
+/// \code
+///  void foo() {
+///if (int i = foobar(); i > 0) {}
+///switch (int i = foobar(); i) {}
+///for (auto& a = get_range(); auto& x : a) {}
+///  }
+///  void bar() {
+///if (foobar() > 0) {}
+///switch (foobar()) {}
+///for (auto& x : get_range()) {}
+///  }
+/// \endcode
+/// ifStmt(hasInitStatement(anything()))
+///   matches the if statement in foo but not in bar.
+/// switchStmt(hasInitStatement(anything()))
+///   matches the switch statement in foo but not in bar.
+/// cxxForRangeStmt(hasInitStatement(anything()))
+///   matches the range for statement in foo but not in bar.
+AST_POLYMORPHIC_MATCHER_P(hasInitStatement,
+  AST_POLYMORPHIC_SUPPORTED_TYPES(IfStmt, SwitchStmt,
+  CXXForRangeStmt),
+  internal::Matcher, InnerMatcher) {
+  const Stmt *Init = Node.getInit();
+  return Init != nullptr && InnerMatcher.matches(*Init, Finder, Builder);
+}
+
 /// Matches the condition expression of an if statement, for loop,
 /// switch statement or conditional operator.
 ///
Index: clang/docs/LibASTMatchersReference.html
===
--- clang/docs/LibASTMatchersReference.html
+++ clang/docs/LibASTMatchersReference.html
@@ -3009,6 +3009,23 @@
 
 
 
+Matcherhttps://clang.llvm.org/doxygen/classclang_1_1Expr.html;>ExprnullPointerConstant
+Matches expressions that resolve to a null pointer constant, such as
+GNU's __null, C++11's nullptr, or C's NULL macro.
+
+Given:
+  void 

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

So I wasn't happy with the vagueness of the else after return check 
implementation. Almost finished rewriting the check to properly handle the if 
statements with condition variables based on scope restrictions and where decls 
are used etc.

  int *varInitAndCondition() {
if (int *X = g(); X != nullptr) {
  return X;
} else {
  h();
  return X;
}
  }
  int *varInitAndConditionUnusedInElseWithDecl() {
int *Y = g();
if (int *X = g(); X != nullptr) {
  return X;
} else {
  int *Y = g();
  h();
}
return Y;
  }

transforms to

  int *varInitAndCondition() {
int *X = g();
if (X != nullptr) {
  return X;
}
h();
return X;
  }
  int *varInitAndConditionUnusedInElseWithDecl() {
int *Y = g();
if (int *X = g(); X != nullptr) {
  return X;
} else {
  int *Y = g();
  h();
}
return Y;
  }

There's a few more test cases but that's the general idea. I'm having a little 
trouble writing the test cases as I can't run them on my windows machine to 
verify they report correctly


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

https://reviews.llvm.org/D71846



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

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM aside from a minor style nit.




Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:4325-4326
+  internal::Matcher, InnerMatcher) {
+  const Stmt *const Init = Node.getInit();
+  return (Init != nullptr && InnerMatcher.matches(*Init, Finder, Builder));
+}

Can drop the top-level `const` qualifier on the pointer (we don't typically do 
that). And can remove the extra parens around the `return` expression. e.g.,
```
const Stmt *Init = Node.getInit();
return Init && InnerMatcher.matches(*Init, Finder, Builder);
```


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

https://reviews.llvm.org/D71846



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

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 235301.
njames93 edited the summary of this revision.
njames93 added a comment.

fixed a spelling mistake in unit tests


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

https://reviews.llvm.org/D71846

Files:
  clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp
  clang/docs/LibASTMatchersReference.html
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/lib/ASTMatchers/Dynamic/Registry.cpp
  clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -1071,6 +1071,35 @@
  LanguageMode::Cxx17OrLater));
 }
 
+TEST(hasInitStatement, MatchesSelectionInitializers) {
+  EXPECT_TRUE(matches("void baz() { if (int i = 1; i > 0) {} }",
+  ifStmt(hasInitStatement(anything())),
+  LanguageMode::Cxx17OrLater));
+  EXPECT_TRUE(notMatches("void baz() { if (int i = 1) {} }",
+ ifStmt(hasInitStatement(anything();
+  EXPECT_TRUE(notMatches("void baz() { if (1 > 0) {} }",
+ ifStmt(hasInitStatement(anything();
+  EXPECT_TRUE(matches(
+  "void baz(int i) { switch (int j = i; j) { default: break; } }",
+  switchStmt(hasInitStatement(anything())), LanguageMode::Cxx17OrLater));
+  EXPECT_TRUE(notMatches("void baz(int i) { switch (i) { default: break; } }",
+ switchStmt(hasInitStatement(anything();
+}
+
+TEST(hasInitStatement, MatchesRangeForInitializers) {
+  EXPECT_TRUE(matches("void baz() {"
+  "int items[] = {};"
+  "for (auto  = items; auto  : arr) {}"
+  "}",
+  cxxForRangeStmt(hasInitStatement(anything())),
+  LanguageMode::Cxx2aOrLater));
+  EXPECT_TRUE(notMatches("void baz() {"
+ "int items[] = {};"
+ "for (auto  : items) {}"
+ "}",
+ cxxForRangeStmt(hasInitStatement(anything();
+}
+
 TEST(TemplateArgumentCountIs, Matches) {
   EXPECT_TRUE(
 matches("template struct C {}; C c;",
Index: clang/lib/ASTMatchers/Dynamic/Registry.cpp
===
--- clang/lib/ASTMatchers/Dynamic/Registry.cpp
+++ clang/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -279,6 +279,7 @@
   REGISTER_MATCHER(hasIndex);
   REGISTER_MATCHER(hasInit);
   REGISTER_MATCHER(hasInitializer);
+  REGISTER_MATCHER(hasInitStatement);
   REGISTER_MATCHER(hasKeywordSelector);
   REGISTER_MATCHER(hasLHS);
   REGISTER_MATCHER(hasLocalQualifiers);
Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -4297,6 +4297,35 @@
   return Node.isConstexpr();
 }
 
+/// Matches selection statements with initializer.
+///
+/// Given:
+/// \code
+///  void foo() {
+///if (int i = foobar(); i > 0) {}
+///switch (int i = foobar(); i) {}
+///for (auto& a = get_range(); auto& x : a) {}
+///  }
+///  void bar() {
+///if (foobar() > 0) {}
+///switch (foobar()) {}
+///for (auto& x : get_range()) {}
+///  }
+/// \endcode
+/// ifStmt(hasInitStatement(anything()))
+///   matches the if statement in foo but not in bar.
+/// switchStmt(hasInitStatement(anything()))
+///   matches the switch statement in foo but not in bar.
+/// cxxForRangeStmt(hasInitStatement(anything()))
+///   matches the range for statement in foo but not in bar.
+AST_POLYMORPHIC_MATCHER_P(hasInitStatement,
+  AST_POLYMORPHIC_SUPPORTED_TYPES(IfStmt, SwitchStmt,
+  CXXForRangeStmt),
+  internal::Matcher, InnerMatcher) {
+  const Stmt *const Init = Node.getInit();
+  return (Init != nullptr && InnerMatcher.matches(*Init, Finder, Builder));
+}
+
 /// Matches the condition expression of an if statement, for loop,
 /// switch statement or conditional operator.
 ///
Index: clang/docs/LibASTMatchersReference.html
===
--- clang/docs/LibASTMatchersReference.html
+++ clang/docs/LibASTMatchersReference.html
@@ -3009,6 +3009,23 @@
 
 
 
+Matcherhttps://clang.llvm.org/doxygen/classclang_1_1Expr.html;>ExprnullPointerConstant
+Matches expressions that resolve to a null pointer constant, such as
+GNU's __null, C++11's nullptr, or C's NULL macro.
+
+Given:
+  void *v1 = NULL;
+  void *v2 = nullptr;
+  void *v3 = __null; // GNU extension
+  char *cp = (char *)0;
+  int *ip = 0;
+ 

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 235300.
njames93 marked an inline comment as done.
njames93 added a comment.

Renamed 'hasInitStorage' to 'hasInitStatement'.
Added support for range for loops in 'hasInitStatement'.
'hasInitStatement' now has a match predicate on the init statement.
ReadabilityElseAfterReturn check now checks the init statement is a decl 
statement before disregarding it.
Added test cases for check and the matcher.
Regenerated the docs for the AST Matchers.


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

https://reviews.llvm.org/D71846

Files:
  clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp
  clang/docs/LibASTMatchersReference.html
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/lib/ASTMatchers/Dynamic/Registry.cpp
  clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -1071,6 +1071,35 @@
  LanguageMode::Cxx17OrLater));
 }
 
+TEST(hasInitStorage, MatchesSelectionInitializers) {
+  EXPECT_TRUE(matches("void baz() { if (int i = 1; i > 0) {} }",
+  ifStmt(hasInitStatement(anything())),
+  LanguageMode::Cxx17OrLater));
+  EXPECT_TRUE(notMatches("void baz() { if (int i = 1) {} }",
+ ifStmt(hasInitStatement(anything();
+  EXPECT_TRUE(notMatches("void baz() { if (1 > 0) {} }",
+ ifStmt(hasInitStatement(anything();
+  EXPECT_TRUE(matches(
+  "void baz(int i) { switch (int j = i; j) { default: break; } }",
+  switchStmt(hasInitStatement(anything())), LanguageMode::Cxx17OrLater));
+  EXPECT_TRUE(notMatches("void baz(int i) { switch (i) { default: break; } }",
+ switchStmt(hasInitStatement(anything();
+}
+
+TEST(hasInitStorage, MatchesRangeForInitializers) {
+  EXPECT_TRUE(matches("void baz() {"
+  "int items[] = {};"
+  "for (auto  = items; auto  : arr) {}"
+  "}",
+  cxxForRangeStmt(hasInitStatement(anything())),
+  LanguageMode::Cxx2aOrLater));
+  EXPECT_TRUE(notMatches("void baz() {"
+ "int items[] = {};"
+ "for (auto  : items) {}"
+ "}",
+ cxxForRangeStmt(hasInitStatement(anything();
+}
+
 TEST(TemplateArgumentCountIs, Matches) {
   EXPECT_TRUE(
 matches("template struct C {}; C c;",
Index: clang/lib/ASTMatchers/Dynamic/Registry.cpp
===
--- clang/lib/ASTMatchers/Dynamic/Registry.cpp
+++ clang/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -279,6 +279,7 @@
   REGISTER_MATCHER(hasIndex);
   REGISTER_MATCHER(hasInit);
   REGISTER_MATCHER(hasInitializer);
+  REGISTER_MATCHER(hasInitStatement);
   REGISTER_MATCHER(hasKeywordSelector);
   REGISTER_MATCHER(hasLHS);
   REGISTER_MATCHER(hasLocalQualifiers);
Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -4297,6 +4297,35 @@
   return Node.isConstexpr();
 }
 
+/// Matches selection statements with initializer.
+///
+/// Given:
+/// \code
+///  void foo() {
+///if (int i = foobar(); i > 0) {}
+///switch (int i = foobar(); i) {}
+///for (auto& a = get_range(); auto& x : a) {}
+///  }
+///  void bar() {
+///if (foobar() > 0) {}
+///switch (foobar()) {}
+///for (auto& x : get_range()) {}
+///  }
+/// \endcode
+/// ifStmt(hasInitStatement(anything()))
+///   matches the if statement in foo but not in bar.
+/// switchStmt(hasInitStatement(anything()))
+///   matches the switch statement in foo but not in bar.
+/// cxxForRangeStmt(hasInitStatement(anything()))
+///   matches the range for statement in foo but not in bar.
+AST_POLYMORPHIC_MATCHER_P(hasInitStatement,
+  AST_POLYMORPHIC_SUPPORTED_TYPES(IfStmt, SwitchStmt,
+  CXXForRangeStmt),
+  internal::Matcher, InnerMatcher) {
+  const Stmt *const Init = Node.getInit();
+  return (Init != nullptr && InnerMatcher.matches(*Init, Finder, Builder));
+}
+
 /// Matches the condition expression of an if statement, for loop,
 /// switch statement or conditional operator.
 ///
Index: clang/docs/LibASTMatchersReference.html
===
--- clang/docs/LibASTMatchersReference.html
+++ clang/docs/LibASTMatchersReference.html
@@ -3009,6 +3009,23 @@
 
 
 

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 235278.
njames93 added a comment.

I added test cases for the matchers and the check, wasn't able to add the 
CXXRangeFor to the matcher as that class uses a different dialect for the 
initializer statement handling. I also couldn't generate the new documentation 
as the script always fails on my machine, even without the changes i have made, 
resulting in the ASTMatcher html being empty


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

https://reviews.llvm.org/D71846

Files:
  clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/lib/ASTMatchers/Dynamic/Registry.cpp
  clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -1071,6 +1071,20 @@
  LanguageMode::Cxx17OrLater));
 }
 
+TEST(hasInitStorage, MatchesSelectionInitializers) {
+  EXPECT_TRUE(matches("void baz() { if (int i = 1; i > 0) {} }",
+  ifStmt(hasInitStorage()), LanguageMode::Cxx17OrLater));
+  EXPECT_TRUE(
+  notMatches("void baz() { if (int i = 1) {} }", ifStmt(hasInitStorage(;
+  EXPECT_TRUE(
+  notMatches("void baz() { if (1 > 0) {} }", ifStmt(hasInitStorage(;
+  EXPECT_TRUE(
+  matches("void baz(int i) { switch (int j = i; j) { default: break; } }",
+  switchStmt(hasInitStorage()), LanguageMode::Cxx17OrLater));
+  EXPECT_TRUE(notMatches("void baz(int i) { switch (i) { default: break; } }",
+ switchStmt(hasInitStorage(;
+}
+
 TEST(TemplateArgumentCountIs, Matches) {
   EXPECT_TRUE(
 matches("template struct C {}; C c;",
Index: clang/lib/ASTMatchers/Dynamic/Registry.cpp
===
--- clang/lib/ASTMatchers/Dynamic/Registry.cpp
+++ clang/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -279,6 +279,7 @@
   REGISTER_MATCHER(hasIndex);
   REGISTER_MATCHER(hasInit);
   REGISTER_MATCHER(hasInitializer);
+  REGISTER_MATCHER(hasInitStorage);
   REGISTER_MATCHER(hasKeywordSelector);
   REGISTER_MATCHER(hasLHS);
   REGISTER_MATCHER(hasLocalQualifiers);
Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -4297,6 +4297,22 @@
   return Node.isConstexpr();
 }
 
+/// Matches selection statements with initializer.
+///
+/// Given:
+/// \code
+///  if (int foo = bar(); bar > 0) {}
+///  switch (int baz = bar(); baz) {}
+/// \endcode
+/// ifStmt(hasInitStorage())
+///   matches the declaration of foo.
+/// switchStmt(hasInitStorage())
+///   matches the declaration of baz.
+AST_POLYMORPHIC_MATCHER(hasInitStorage,
+AST_POLYMORPHIC_SUPPORTED_TYPES(IfStmt, SwitchStmt)) {
+  return Node.hasInitStorage();
+}
+
 /// Matches the condition expression of an if statement, for loop,
 /// switch statement or conditional operator.
 ///
Index: clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s readability-else-after-return %t -- -- -fexceptions
+// RUN: %check_clang_tidy %s readability-else-after-return %t -- -- -fexceptions -std=c++17
 
 namespace std {
 struct string {
@@ -117,3 +117,12 @@
 return x;
   }
 }
+
+int *init_and_condition() {
+  if (int *x = g(); x != nullptr) {
+return x;
+  } else {
+h();
+return x;
+  }
+}
Index: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
@@ -26,12 +26,14 @@
   compoundStmt(forEach(
   ifStmt(unless(isConstexpr()),
  // FIXME: Explore alternatives for the
- // `if (T x = ...) {... return; } else {  }`
- // pattern:
+ // `if (T x = ...) {... return; } else {  }` and
+ // 'if (T x = ...; cond) {... return; } else { use  }'
+ // patterns:
  //   * warn, but don't fix;
  //   * fix by pulling out the variable declaration out of
  // the condition.
  unless(hasConditionVariableStatement(anything())),
+ 

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

Thanks for the pointers, I'll amend those changes and update the patch, thanks 
for pointing it out for me. Probably be after boxing day though


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71846



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

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman requested changes to this revision.
aaron.ballman added a comment.
This revision now requires changes to proceed.

Thank you for this!

There's some missing test coverage for the changes to the clang-tidy check and 
for the new matcher. Also, the new matcher should be registered in 
`clang\lib\ASTMatchers\Dynamic\Registry.cpp` (in alphabetical order). Oh, and 
you should also run `clang\docs\tools\dump_ast_matchers.py` to regenerate the 
AST matcher documentation.




Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:4300
 
+/// Matches selection statements with initializer
+///

Missing a full stop at the end of the sentence.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71846



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

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

It does support the new range for loop, I'll add in a case for that in the AST 
matcher


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71846



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

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

[Malcolm Parsons via Phabricator via cfe-commits]

malcolm.parsons added a comment.

Does Clang support C++20's range-based for statements with initializer?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71846



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