aaron.ballman closed this revision.
aaron.ballman added a comment.
I've commit in r301185, thank you!
Repository:
rL LLVM
https://reviews.llvm.org/D31542
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
joshz added a comment.
In https://reviews.llvm.org/D31542#734809, @aaron.ballman wrote:
> In https://reviews.llvm.org/D31542#734455, @joshz wrote:
>
> > Are there any further changes I should make, or is this good to submit now?
> >
> > Thanks!
>
>
> This still LGTM, so it's good to submit. Do
aaron.ballman added a comment.
In https://reviews.llvm.org/D31542#734455, @joshz wrote:
> Are there any further changes I should make, or is this good to submit now?
>
> Thanks!
This still LGTM, so it's good to submit. Do you still need someone to land the
patch for you?
Repository:
rL
joshz added a comment.
Are there any further changes I should make, or is this good to submit now?
Thanks!
Repository:
rL LLVM
https://reviews.llvm.org/D31542
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
joshz updated this revision to Diff 95833.
joshz marked 3 inline comments as done.
joshz added a comment.
Clean up IsBinaryOrTernary
Repository:
rL LLVM
https://reviews.llvm.org/D31542
Files:
clang-tidy/readability/ContainerSizeEmptyCheck.cpp
clang-tidy/utils/ASTUtils.cpp
aaron.ballman added inline comments.
Comment at: clang-tidy/utils/ASTUtils.cpp:28
+bool IsBinaryOrTernary(const Expr *expr) {
+ if (clang::isa(expr->IgnoreImpCasts()) ||
+ clang::isa(expr->IgnoreImpCasts())) {
Rather than call `IgnoreImpCasts()` three
joshz updated this revision to Diff 95655.
joshz added a comment.
Updated per some suggestions by sbenza@ on dealing with the parentheses;
IsBinaryOrTernary is based on a function he wrote at Google.
PTAL.
Repository:
rL LLVM
https://reviews.llvm.org/D31542
Files:
alexfh accepted this revision.
alexfh added a comment.
LG with one nit.
Comment at: clang-tidy/readability/ContainerSizeEmptyCheck.cpp:209
+ "the 'empty' method should be used to check "
+ "for emptiness instead of comparing to an empty object.")
+<<
joshz added a comment.
I don't believe I have access to commit this revision myself; can someone
please do it for me?
Thanks! :-)
Repository:
rL LLVM
https://reviews.llvm.org/D31542
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
joshz added a comment.
Thanks, Aaron!
Repository:
rL LLVM
https://reviews.llvm.org/D31542
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM
Repository:
rL LLVM
https://reviews.llvm.org/D31542
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
joshz updated this revision to Diff 95488.
joshz marked an inline comment as done.
joshz edited the summary of this revision.
joshz added a comment.
Resolved the bug, with a slightly modified version of Aaron's suggestion. (It
will suggest parens for anything that wasn't just a DeclRefExpr,
aaron.ballman added a comment.
> ... which has the wrong precedence; an extra set of parens is necessary.
> However, I don't want to add a set of parens if it isn't necessary. ...
One way to handle this is to not suggest the fixit if the container part of the
equality check is not a
joshz added a comment.
Hey there, reviewers.
Any chance you can take a look at this change?
Thanks! :-)
Repository:
rL LLVM
https://reviews.llvm.org/D31542
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
14 matches
Mail list logo
