aaron.ballman accepted this revision.
aaron.ballman added a comment.
LGTM! We may want to consider BinaryConditionalOperator as well, but that can
be in as a separate patch.
================
Comment at: clang-tidy/modernize/UseBoolLiteralsCheck.cpp:51-52
@@ -34,4 +50,4 @@
void UseBoolLiteralsCheck::check(const MatchFinder::MatchResult &Result) {
- const auto *Literal = Result.Nodes.getNodeAs<IntegerLiteral>("literal");
- const auto *Cast = Result.Nodes.getNodeAs<Expr>("cast");
- bool LiteralBooleanValue = Literal->getValue().getBoolValue();
+ for (const auto &BindingName :
+ {"literal", "trueBranchLiteral", "falseBranchLiteral"}) {
+ const auto *Literal = Result.Nodes.getNodeAs<IntegerLiteral>(BindingName);
----------------
omtcyfz wrote:
> aaron.ballman wrote:
> > omtcyfz wrote:
> > > aaron.ballman wrote:
> > > > omtcyfz wrote:
> > > > > aaron.ballman wrote:
> > > > > > omtcyfz wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > Any reason not to name the bind "literal" in all three cases?
> > > > > > > > That eliminates the need for the loop entirely, since `check()`
> > > > > > > > will trigger for each instance of a match.
> > > > > > > It doesn't make sense to try binding both `TrueExpression` and
> > > > > > > `FalseExpression` literals to a single value.
> > > > > > Why? In all three cases, you don't care what matched, just that
> > > > > > *some* case is matched. None of the logic in `check()` relies on
> > > > > > which part of the expression is matched.
> > > > > Well, in case of second matcher I may have **two** literals matched
> > > > > upon triggering. I don't understand how I could possibly get **two**
> > > > > literals bound to **one** value after **one** matcher got triggered.
> > > > >
> > > > > Am I missing something?
> > > > One matcher isn't what's getting triggered then, is it? I could be
> > > > wrong on this point, but I thought that in that case, `check()` would
> > > > be called twice, once for each literal. Is that not the case?
> > > From what I understand about how matchers work, it is not. Plus, I
> > > checked (just named everything `"literal"` and removed `for (const auto
> > > ...` just to double check in case I was wrong.
> > Thank you for checking -- I learned something new! :-)
> No problem :)
>
> One case in which your solution would work is splitting second matcher into
> two: one with `hasTrueExpression` and one with `hasFalseExpression`, then
> binding to `"literal"` would work. But it'll be inefficient and three names
> of bindings don't seem like too much hardcoding after all.
I agree, that would be a step backwards.
https://reviews.llvm.org/D23243
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
