[PATCH] D82824: [clang-tidy] Added option to readability-else-after-return

2020-06-30 Thread Nathan James via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rGa06a5ed97800: [clang-tidy] Added option to 
readability-else-after-return (authored by njames93).

Changed prior to commit:
  https://reviews.llvm.org/D82824?vs=274473=274569#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82824

Files:
  clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
  clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-else-after-return.rst
  
clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return-no-cond-var-refactor.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return-no-cond-var-refactor.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return-no-cond-var-refactor.cpp
@@ -0,0 +1,42 @@
+// RUN: %check_clang_tidy %s readability-else-after-return %t -- \
+// RUN: -config='{CheckOptions: [ \
+// RUN: {key: readability-else-after-return.WarnOnConditionVariables, value: false}, \
+// RUN: ]}'
+
+bool foo(int Y) {
+  // Excess scopes are here so that the check would have to opportunity to
+  // refactor the variable out of the condition.
+
+  // Expect warnings here as we don't need to move declaration of 'X' out of the
+  // if condition as its not used in the else.
+  {
+if (int X = Y)
+  return X < 0;
+else
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+  return false;
+  }
+  {
+if (int X = Y; X)
+  return X < 0;
+else
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+  return false;
+  }
+
+  // Expect no warnings for these cases, as even though its safe to move
+  // declaration of 'X' out of the if condition, that has been disabled
+  // by the options.
+  {
+if (int X = Y)
+  return false;
+else
+  return X < 0;
+  }
+  {
+if (int X = Y; X)
+  return false;
+else
+  return X < 0;
+  }
+}
Index: clang-tools-extra/docs/clang-tidy/checks/readability-else-after-return.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/readability-else-after-return.rst
+++ clang-tools-extra/docs/clang-tidy/checks/readability-else-after-return.rst
@@ -59,6 +59,21 @@
   }
 }
 
+Options
+---
+
+.. option:: WarnOnUnfixable
+
+   When `true`, emit a warning for cases where the check can't output a 
+   Fix-It. These can occur with declarations inside the ``else`` branch that
+   would have an extended lifetime if the ``else`` branch was removed.
+   Default value is `true`.
+
+.. option:: WarnOnConditionVariables
+
+   When `true`, the check will attempt to refactor a variable defined inside
+   the condition of the ``if`` statement that is used in the ``else`` branch
+   defining them just before the ``if`` statement. This can only be done if 
+   the ``if`` statement is the last statement in its parents scope.
+   Default value is `true`.
 
-This check helps to enforce this `LLVM Coding Standards recommendation
-`_.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -200,6 +200,11 @@
 
   Now checks ``std::basic_string_view`` by default.
 
+- Improved :doc:`readability-else-after-return
+  ` check now supports a 
+  `WarnOnConditionVariables` option to control whether to refactor condition
+  variables where possible.
+
 - Improved :doc:`readability-identifier-naming
   ` check.
 
Index: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h
===
--- clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h
+++ clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h
@@ -28,6 +28,7 @@
 
 private:
   const bool WarnOnUnfixable;
+  const bool WarnOnConditionVariables;
 };
 
 } // namespace readability
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,6 +26,7 @@
 static const char ThrowStr[] = "throw";
 static const char WarningMessage[] = "do not use 'else' after '%0'";
 static const char WarnOnUnfixableStr[] = "WarnOnUnfixable";
+static const char WarnOnConditionVariablesStr[] = "WarnOnConditionVariables";
 
 const DeclRefExpr *findUsage(const Stmt *Node, int64_t DeclIdentifier) {
   

[PATCH] D82824: [clang-tidy] Added option to readability-else-after-return

2020-06-30 Thread 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 once the documentation nit is fixed up.




Comment at: 
clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp:193
   if (checkConditionVarUsageInElse(If) != nullptr) {
+if (!WarnOnConditionVariables)
+  return;

njames93 wrote:
> aaron.ballman wrote:
> > njames93 wrote:
> > > aaron.ballman wrote:
> > > > Would it make sense to hoist this into the previous `if` statement so 
> > > > we don't bother checking the condition var use in the first place if 
> > > > we're just going to ignore the results?
> > > That wouldn't work, we need to see if there is a condition variable that 
> > > needs refactoring first before we can disregard it, Or am I missing 
> > > something?
> > I was suggesting:
> > ```
> > if (WarnOnConditionVariables && checkConditionVarUsageInElse(If)) {
> >  ...
> > }
> > if (WarnOnConditionVariables && checkInitDeclUsageInElse(If) {
> > ...
> > }
> > ```
> > The effect is that we don't bother checking for the situations we weren't 
> > going to warn about anyway. But maybe I'm the one missing something?
> I think you may be this time. The reason being those `if` statements have a 
> `return` at the end. If I followed how you have it layed out, those returns 
> would never hit regardless of whether there was a condition variable that 
> needed handling or not.
Oh! So you're talking about the case where `IsLastInScope` and 
`WarnOnUnfixable` are both `false`, okay I see that now. I think the logic 
could still be rearranged to avoid doing work you're just going to immediately 
throw away, but I don't think it's critical (or really needed for this patch).



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-else-after-return.rst:74
+
+   When `true`, The check will attempt to refactor a variable defined inside
+   the condition of the ``if`` statement that is used in the ``else`` branch.

aaron.ballman wrote:
> njames93 wrote:
> > aaron.ballman wrote:
> > > The -> the
> > > 
> > > I'm a bit unclear on what "attempt to refactor" means -- I sort of 
> > > understand it to mean that if this option is true then the check will not 
> > > produce a fix-it for variables defined inside the condition of the if 
> > > statement that is used in the else branch, but will produce a diagnostic. 
> > > However, the check behavior seems to also remove the diagnostic in this 
> > > case (not just the fix-it), so I'm not certain I'm reading this right.
> > Good spot, the check behaviour is also removing the diagnostic as well as 
> > the fix it.
> > That behaviour should probably be changed to removing the Fix-It when this 
> > option is `false`, but then diagnostic behaviour should follow what 
> > `WarnOnUnfixable` dictates.
> I think that makes sense. Then the option can be renamed to remove the 
> "Refactor" bit and probably be something more like `WarnOnConditionVariables` 
> or something?
Still have to fix the capitalization issues with `The` and `Emit`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82824



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


[PATCH] D82824: [clang-tidy] Added option to readability-else-after-return

2020-06-30 Thread Nathan James via Phabricator via cfe-commits
njames93 marked an inline comment as done.
njames93 added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp:193
   if (checkConditionVarUsageInElse(If) != nullptr) {
+if (!WarnOnConditionVariables)
+  return;

aaron.ballman wrote:
> njames93 wrote:
> > aaron.ballman wrote:
> > > Would it make sense to hoist this into the previous `if` statement so we 
> > > don't bother checking the condition var use in the first place if we're 
> > > just going to ignore the results?
> > That wouldn't work, we need to see if there is a condition variable that 
> > needs refactoring first before we can disregard it, Or am I missing 
> > something?
> I was suggesting:
> ```
> if (WarnOnConditionVariables && checkConditionVarUsageInElse(If)) {
>  ...
> }
> if (WarnOnConditionVariables && checkInitDeclUsageInElse(If) {
> ...
> }
> ```
> The effect is that we don't bother checking for the situations we weren't 
> going to warn about anyway. But maybe I'm the one missing something?
I think you may be this time. The reason being those `if` statements have a 
`return` at the end. If I followed how you have it layed out, those returns 
would never hit regardless of whether there was a condition variable that 
needed handling or not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82824



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


[PATCH] D82824: [clang-tidy] Added option to readability-else-after-return

2020-06-30 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp:193
   if (checkConditionVarUsageInElse(If) != nullptr) {
+if (!WarnOnConditionVariables)
+  return;

njames93 wrote:
> aaron.ballman wrote:
> > Would it make sense to hoist this into the previous `if` statement so we 
> > don't bother checking the condition var use in the first place if we're 
> > just going to ignore the results?
> That wouldn't work, we need to see if there is a condition variable that 
> needs refactoring first before we can disregard it, Or am I missing something?
I was suggesting:
```
if (WarnOnConditionVariables && checkConditionVarUsageInElse(If)) {
 ...
}
if (WarnOnConditionVariables && checkInitDeclUsageInElse(If) {
...
}
```
The effect is that we don't bother checking for the situations we weren't going 
to warn about anyway. But maybe I'm the one missing something?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82824



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


[PATCH] D82824: [clang-tidy] Added option to readability-else-after-return

2020-06-30 Thread Nathan James via Phabricator via cfe-commits
njames93 marked an inline comment as done.
njames93 added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp:193
   if (checkConditionVarUsageInElse(If) != nullptr) {
+if (!WarnOnConditionVariables)
+  return;

aaron.ballman wrote:
> Would it make sense to hoist this into the previous `if` statement so we 
> don't bother checking the condition var use in the first place if we're just 
> going to ignore the results?
That wouldn't work, we need to see if there is a condition variable that needs 
refactoring first before we can disregard it, Or am I missing something?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82824



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


[PATCH] D82824: [clang-tidy] Added option to readability-else-after-return

2020-06-30 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp:193
   if (checkConditionVarUsageInElse(If) != nullptr) {
+if (!WarnOnConditionVariables)
+  return;

Would it make sense to hoist this into the previous `if` statement so we don't 
bother checking the condition var use in the first place if we're just going to 
ignore the results?



Comment at: 
clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp:228
   if (checkInitDeclUsageInElse(If) != nullptr) {
+if (!WarnOnConditionVariables)
+  return;

Same here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82824



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


[PATCH] D82824: [clang-tidy] Added option to readability-else-after-return

2020-06-30 Thread Nathan James via Phabricator via cfe-commits
njames93 updated this revision to Diff 274473.
njames93 added a comment.

Renamed new option to WarnOnConditionVariables


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82824

Files:
  clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
  clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-else-after-return.rst
  
clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return-no-cond-var-refactor.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return-no-cond-var-refactor.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return-no-cond-var-refactor.cpp
@@ -0,0 +1,42 @@
+// RUN: %check_clang_tidy %s readability-else-after-return %t -- \
+// RUN: -config='{CheckOptions: [ \
+// RUN: {key: readability-else-after-return.WarnOnConditionVariables, value: false}, \
+// RUN: ]}'
+
+bool foo(int Y) {
+  // Excess scopes are here so that the check would have to opportunity to
+  // refactor the variable out of the condition.
+
+  // Expect warnings here as we don't need to move declaration of 'X' out of the
+  // if condition as its not used in the else.
+  {
+if (int X = Y)
+  return X < 0;
+else
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+  return false;
+  }
+  {
+if (int X = Y; X)
+  return X < 0;
+else
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+  return false;
+  }
+
+  // Expect no warnings for these cases, as even though its safe to move
+  // declaration of 'X' out of the if condition, that has been disabled
+  // by the options.
+  {
+if (int X = Y)
+  return false;
+else
+  return X < 0;
+  }
+  {
+if (int X = Y; X)
+  return false;
+else
+  return X < 0;
+  }
+}
Index: clang-tools-extra/docs/clang-tidy/checks/readability-else-after-return.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/readability-else-after-return.rst
+++ clang-tools-extra/docs/clang-tidy/checks/readability-else-after-return.rst
@@ -59,6 +59,21 @@
   }
 }
 
+Options
+---
+
+.. option:: WarnOnUnfixable
+
+   When `true`, Emit a warning for cases where the check can't output a 
+   Fix-It. These can occur with declarations inside the ``else`` branch that
+   would have an extended lifetime if the ``else`` branch was removed.
+   Default value is `true`.
+
+.. option:: WarnOnConditionVariables
+
+   When `true`, The check will attempt to refactor a variable defined inside
+   the condition of the ``if`` statement that is used in the ``else`` branch
+   defining them just before the ``if`` statement. This can only be done if 
+   the ``if`` statement is the last statement in its parents scope.
+   Default value is `true`.
 
-This check helps to enforce this `LLVM Coding Standards recommendation
-`_.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -195,6 +195,11 @@
 Changes in existing checks
 ^^
 
+- Improved :doc:`readability-else-after-return
+  ` check now supports a 
+  `WarnOnConditionVariables` option to control whether to refactor condition
+  variables where possible.
+
 - Improved :doc:`readability-identifier-naming
   ` check.
 
Index: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h
===
--- clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h
+++ clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h
@@ -28,6 +28,7 @@
 
 private:
   const bool WarnOnUnfixable;
+  const bool WarnOnConditionVariables;
 };
 
 } // namespace readability
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,6 +26,7 @@
 static const char ThrowStr[] = "throw";
 static const char WarningMessage[] = "do not use 'else' after '%0'";
 static const char WarnOnUnfixableStr[] = "WarnOnUnfixable";
+static const char WarnOnConditionVariablesStr[] = "WarnOnConditionVariables";
 
 const DeclRefExpr *findUsage(const Stmt *Node, int64_t DeclIdentifier) {
   if (!Node)
@@ -138,10 +139,13 @@
 ElseAfterReturnCheck::ElseAfterReturnCheck(StringRef Name,

[PATCH] D82824: [clang-tidy] Added option to readability-else-after-return

2020-06-30 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-else-after-return.rst:74
+
+   When `true`, The check will attempt to refactor a variable defined inside
+   the condition of the ``if`` statement that is used in the ``else`` branch.

njames93 wrote:
> aaron.ballman wrote:
> > The -> the
> > 
> > I'm a bit unclear on what "attempt to refactor" means -- I sort of 
> > understand it to mean that if this option is true then the check will not 
> > produce a fix-it for variables defined inside the condition of the if 
> > statement that is used in the else branch, but will produce a diagnostic. 
> > However, the check behavior seems to also remove the diagnostic in this 
> > case (not just the fix-it), so I'm not certain I'm reading this right.
> Good spot, the check behaviour is also removing the diagnostic as well as the 
> fix it.
> That behaviour should probably be changed to removing the Fix-It when this 
> option is `false`, but then diagnostic behaviour should follow what 
> `WarnOnUnfixable` dictates.
I think that makes sense. Then the option can be renamed to remove the 
"Refactor" bit and probably be something more like `WarnOnConditionVariables` 
or something?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82824



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


[PATCH] D82824: [clang-tidy] Added option to readability-else-after-return

2020-06-30 Thread Nathan James via Phabricator via cfe-commits
njames93 marked an inline comment as done.
njames93 added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-else-after-return.rst:74
+
+   When `true`, The check will attempt to refactor a variable defined inside
+   the condition of the ``if`` statement that is used in the ``else`` branch.

aaron.ballman wrote:
> The -> the
> 
> I'm a bit unclear on what "attempt to refactor" means -- I sort of understand 
> it to mean that if this option is true then the check will not produce a 
> fix-it for variables defined inside the condition of the if statement that is 
> used in the else branch, but will produce a diagnostic. However, the check 
> behavior seems to also remove the diagnostic in this case (not just the 
> fix-it), so I'm not certain I'm reading this right.
Good spot, the check behaviour is also removing the diagnostic as well as the 
fix it.
That behaviour should probably be changed to removing the Fix-It when this 
option is `false`, but then diagnostic behaviour should follow what 
`WarnOnUnfixable` dictates.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82824



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


[PATCH] D82824: [clang-tidy] Added option to readability-else-after-return

2020-06-30 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-else-after-return.rst:67
+
+   When `true`, Emit a warning for cases where the check can't output a 
+   Fix-It. These can occur with declarations inside the ``else`` branch that

Emit -> emit



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-else-after-return.rst:74
+
+   When `true`, The check will attempt to refactor a variable defined inside
+   the condition of the ``if`` statement that is used in the ``else`` branch.

The -> the

I'm a bit unclear on what "attempt to refactor" means -- I sort of understand 
it to mean that if this option is true then the check will not produce a fix-it 
for variables defined inside the condition of the if statement that is used in 
the else branch, but will produce a diagnostic. However, the check behavior 
seems to also remove the diagnostic in this case (not just the fix-it), so I'm 
not certain I'm reading this right.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82824



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


[PATCH] D82824: [clang-tidy] Added option to readability-else-after-return

2020-06-30 Thread Nathan James via Phabricator via cfe-commits
njames93 updated this revision to Diff 274353.
njames93 added a comment.

Address doc backticks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82824

Files:
  clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
  clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-else-after-return.rst
  
clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return-no-cond-var-refactor.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return-no-cond-var-refactor.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return-no-cond-var-refactor.cpp
@@ -0,0 +1,42 @@
+// RUN: %check_clang_tidy %s readability-else-after-return %t -- \
+// RUN: -config='{CheckOptions: [ \
+// RUN: {key: readability-else-after-return.RefactorConditionVariables, value: false}, \
+// RUN: ]}'
+
+bool foo(int Y) {
+  // Excess scopes are here so that the check would have to opportunity to
+  // refactor the variable out of the condition.
+
+  // Expect warnings here as we don't need to move declaration of 'X' out of the
+  // if condition as its not used in the else.
+  {
+if (int X = Y)
+  return X < 0;
+else
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+  return false;
+  }
+  {
+if (int X = Y; X)
+  return X < 0;
+else
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+  return false;
+  }
+
+  // Expect no warnings for these cases, as even though its safe to move
+  // declaration of 'X' out of the if condition, that has been disabled
+  // by the options.
+  {
+if (int X = Y)
+  return false;
+else
+  return X < 0;
+  }
+  {
+if (int X = Y; X)
+  return false;
+else
+  return X < 0;
+  }
+}
Index: clang-tools-extra/docs/clang-tidy/checks/readability-else-after-return.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/readability-else-after-return.rst
+++ clang-tools-extra/docs/clang-tidy/checks/readability-else-after-return.rst
@@ -59,6 +59,19 @@
   }
 }
 
+Options
+---
+
+.. option:: WarnOnUnfixable
+
+   When `true`, Emit a warning for cases where the check can't output a 
+   Fix-It. These can occur with declarations inside the ``else`` branch that
+   would have an extended lifetime if the ``else`` branch was removed.
+   Default value is `true`.
+
+.. option:: RefactorConditionVariables
+
+   When `true`, The check will attempt to refactor a variable defined inside
+   the condition of the ``if`` statement that is used in the ``else`` branch.
+   Default value is `true`.
 
-This check helps to enforce this `LLVM Coding Standards recommendation
-`_.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -195,6 +195,11 @@
 Changes in existing checks
 ^^
 
+- Improved :doc:`readability-else-after-return
+  ` check now supports a 
+  `RefactorConditionVariables` option to control whether to refactor condition
+  variables where possible.
+
 - Improved :doc:'readability-identifier-naming
   ` check.
 
Index: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h
===
--- clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h
+++ clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h
@@ -28,6 +28,7 @@
 
 private:
   const bool WarnOnUnfixable;
+  const bool RefactorConditionVariables;
 };
 
 } // namespace readability
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,6 +26,8 @@
 static const char ThrowStr[] = "throw";
 static const char WarningMessage[] = "do not use 'else' after '%0'";
 static const char WarnOnUnfixableStr[] = "WarnOnUnfixable";
+static const char RefactorConditionVariablesStr[] =
+"RefactorConditionVariables";
 
 const DeclRefExpr *findUsage(const Stmt *Node, int64_t DeclIdentifier) {
   if (!Node)
@@ -138,10 +140,14 @@
 ElseAfterReturnCheck::ElseAfterReturnCheck(StringRef Name,
ClangTidyContext *Context)
 : ClangTidyCheck(Name, Context),
-  WarnOnUnfixable(Options.get(WarnOnUnfixableStr, true)) {}
+  

[PATCH] D82824: [clang-tidy] Added option to readability-else-after-return

2020-06-29 Thread Eugene Zelenko via Phabricator via cfe-commits
Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:200
+  ` check now supports a 
+  ``RefactorConditionVariables`` option to control whether to refactor 
condition
+  variables where possible.

Please use single back-ticks for option name.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-else-after-return.rst:67
+
+   When ``true``, Emit a warning for cases where the check can't output a 
+   Fix-It. These can occur with declarations inside the ``else`` branch that

Please use single back-ticks for option values. Same below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82824



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


[PATCH] D82824: [clang-tidy] Added option to readability-else-after-return

2020-06-29 Thread Nathan James via Phabricator via cfe-commits
njames93 created this revision.
njames93 added reviewers: aaron.ballman, alexfh, gribozavr2.
Herald added subscribers: cfe-commits, xazax.hun.
Herald added a project: clang.

Added a 'RefactorConditionVariables' option to control how the check handles 
condition variables


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D82824

Files:
  clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
  clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-else-after-return.rst
  
clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return-no-cond-var-refactor.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return-no-cond-var-refactor.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return-no-cond-var-refactor.cpp
@@ -0,0 +1,42 @@
+// RUN: %check_clang_tidy %s readability-else-after-return %t -- \
+// RUN: -config='{CheckOptions: [ \
+// RUN: {key: readability-else-after-return.RefactorConditionVariables, value: false}, \
+// RUN: ]}'
+
+bool foo(int Y) {
+  // Excess scopes are here so that the check would have to opportunity to
+  // refactor the variable out of the condition.
+
+  // Expect warnings here as we don't need to move declaration of 'X' out of the
+  // if condition as its not used in the else.
+  {
+if (int X = Y)
+  return X < 0;
+else
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+  return false;
+  }
+  {
+if (int X = Y; X)
+  return X < 0;
+else
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+  return false;
+  }
+
+  // Expect no warnings for these cases, as even though its safe to move
+  // declaration of 'X' out of the if condition, that has been disabled
+  // by the options.
+  {
+if (int X = Y)
+  return false;
+else
+  return X < 0;
+  }
+  {
+if (int X = Y; X)
+  return false;
+else
+  return X < 0;
+  }
+}
Index: clang-tools-extra/docs/clang-tidy/checks/readability-else-after-return.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/readability-else-after-return.rst
+++ clang-tools-extra/docs/clang-tidy/checks/readability-else-after-return.rst
@@ -59,6 +59,19 @@
   }
 }
 
+Options
+---
+
+.. option:: WarnOnUnfixable
+
+   When ``true``, Emit a warning for cases where the check can't output a 
+   Fix-It. These can occur with declarations inside the ``else`` branch that
+   would have an extended lifetime if the ``else`` branch was removed.
+   Default value is ``true``.
+
+.. option:: RefactorConditionVariables
+
+   When ``true``, The check will attempt to refactor a variable defined inside
+   the condition of the if statement that is used in the else branch.
+   Default value is ``true``.
 
-This check helps to enforce this `LLVM Coding Standards recommendation
-`_.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -195,6 +195,11 @@
 Changes in existing checks
 ^^
 
+- Improved :doc:`readability-else-after-return
+  ` check now supports a 
+  ``RefactorConditionVariables`` option to control whether to refactor condition
+  variables where possible.
+
 - Improved :doc:'readability-identifier-naming
   ` check.
 
Index: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h
===
--- clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h
+++ clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h
@@ -28,6 +28,7 @@
 
 private:
   const bool WarnOnUnfixable;
+  const bool RefactorConditionVariables;
 };
 
 } // namespace readability
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,6 +26,8 @@
 static const char ThrowStr[] = "throw";
 static const char WarningMessage[] = "do not use 'else' after '%0'";
 static const char WarnOnUnfixableStr[] = "WarnOnUnfixable";
+static const char RefactorConditionVariablesStr[] =
+"RefactorConditionVariables";
 
 const DeclRefExpr *findUsage(const Stmt *Node, int64_t DeclIdentifier) {
   if (!Node)
@@ -138,10 +140,14 @@
 ElseAfterReturnCheck::ElseAfterReturnCheck(StringRef Name,
ClangTidyContext *Context)
 :