[PATCH] D82824: [clang-tidy] Added option to readability-else-after-return
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&id=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
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
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
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
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
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
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,
ClangTidyCont
[PATCH] D82824: [clang-tidy] Added option to readability-else-after-return
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
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
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
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)) {}
+ WarnOnUnfixable(Options.get(W
[PATCH] D82824: [clang-tidy] Added option to readability-else-after-return
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
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)
: ClangTidyCheck(Name,
