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
