aaron.ballman added inline comments.

================
Comment at: 
clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp:56
+AST_MATCHER(Expr, usedInBooleanContext) {
+  std::string exprName = "__booleanContextExpr";
+  auto result =
----------------



================
Comment at: 
clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp:57
+  std::string exprName = "__booleanContextExpr";
+  auto result =
+      expr(expr().bind(exprName),
----------------



================
Comment at: 
clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp:84
+          .matches(Node, Finder, Builder);
+  Builder->removeBindings([this, exprName](const BoundNodesMap &Nodes) {
+    return Nodes.getNode(exprName).getNodeKind().isNone();
----------------
Might as well drop the `this` from here.


================
Comment at: 
clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp:67
+              
hasParent(forStmt(hasCondition(expr(equalsBoundNode(exprName))))),
+              hasParent(varDecl(hasType(booleanType()))),
+              hasParent(
----------------
steveire wrote:
> aaron.ballman wrote:
> > If this matters for var decls, what about field decls in a structure?
> Not sure. What would this look like in a test?
I was thinking this one:
```
struct S {
  std::vector<int> C;
  bool B = C.size();
};
```
but that seems to be handled properly already.


================
Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-container-size-empty.cpp:493
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be 
used to check for emptiness instead of 'size' [readability-container-size-empty]
+  // CHECK-MESSAGES: :9:8: note: method 'vector'::empty() defined here
+  // CHECK-FIXES: {{^  }}if (!v.empty()){{$}}
----------------
steveire wrote:
> aaron.ballman wrote:
> > The quoting here is unfortunate. It's not part of your patch, but it could 
> > be solved by calling `Container->getName()` and manually quoting the note 
> > text if you wanted to hit it in a follow-up (I'd consider that an NFC 
> > change, no need to review). You could also address it as part of this patch 
> > if you felt like it.
> That will be a follow-up.
SGTM, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91302

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

Reply via email to