aaron.ballman added inline comments.

================
Comment at: 
clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp:26-28
+  // The first argument of an overloaded member operator is the implicit object
+  // argument of the method which should not be matched against a parameter, so
+  // we skip over it here.
----------------
Isn't this true for any non-static member function? e.g., should the matcher be 
looking at `cxxOperatorCallExpr()` at all?


================
Comment at: 
clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp:61
+          anyOf(
+              hasParent(explicitCastExpr(hasDestinationType(booleanType()))),
+              hasParent(ifStmt(hasCondition(expr(equalsBoundNode(exprName))))),
----------------
I take it we can't do `hasParent(anyOf(...))` instead?


================
Comment at: 
clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp:67
+              
hasParent(forStmt(hasCondition(expr(equalsBoundNode(exprName))))),
+              hasParent(varDecl(hasType(booleanType()))),
+              hasParent(
----------------
If this matters for var decls, what about field decls in a structure?


================
Comment at: 
clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp:69
+              hasParent(
+                  parenListExpr(hasParent(varDecl(hasType(booleanType()))))),
+              hasParent(parenExpr(hasParent(
----------------
Same question here.


================
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()){{$}}
----------------
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.


================
Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-container-size-empty.cpp:631
+};
+
+void instantiator() {
----------------
Some other test cases that may be of interest:
```
class C {
  bool B;
public:
  C(const std::vector<int> &C) : B(C.size()) {}
};

struct S {
  std::vector<int> C;
  bool B = C.size();
};

int func(const std::vector<int> &C) {
  return C.size() ? 0 : 1;
}

struct Lazy {
  constexpr unsigned size() const { return 0; }
  constexpr bool empty() const { return true; }
};

constexpr Lazy L;
static_assert(!L.size(), ""); // This one should get converted

struct S {
  void func() noexcept(L.size()); // This one should not
};
```


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