alexfh added a comment. Thank you for the patch!
In https://reviews.llvm.org/D24349#537500, @aaron.ballman wrote: > In https://reviews.llvm.org/D24349#537350, @Eugene.Zelenko wrote: > > > Probably check should have options to extend list of containers and also to > > assume all classes with integer type size() const and bool empty() const as > > containers. It may be not trivial to find out all custom containers and > > last option will be helpful to assemble such list. > > > I think this should actually have two options: one is a list of > default-checked containers (default is the list of > STL containers we previously had), and the other is a Boolean option for > guessing at what might be a > container based on function signature (default is true). This gives people a > way to silence the diagnostic > while still being useful. While I understand your concerns, I would suggest to wait for actual bug reports before introducing more options. The duck typing approach worked really well for readability-redundant-smartptr-get and I expect it to work equally well here. The constraints checked here are pretty strict and should make the check safe. > I would document what function signatures we use as our heuristic, but note > that the function signatures we care about may change (for instance, we may > decide that const-qualification is unimportant, or that we want to require > begin()/end() functions, etc). I think `<standard integer type> size() const` > and `bool empty() const` are a reasonable heuristic for a first pass. Yep, documentation should be updated accordingly. ================ Comment at: clang-tidy/readability/ContainerSizeEmptyCheck.cpp:33 @@ +32,3 @@ + const auto validContainer = namedDecl( + has(functionDecl( + isPublic(), hasName("size"), returns(isInteger()), ---------------- aaron.ballman wrote: > omtcyfz wrote: > > Thank you! > > > > Blacklisted these types. > > > > Actually, I believe if someone has `bool size()` in their codebase there's > > still a problem... > > Blacklisted these types. > > I'm still not certain this is correct. For instance, if someone wants to do > `uint8_t size() const;`, we shouldn't punish them because `uint8_t` happens > to be `unsigned char` for their platform. But you are correct that we don't > want this to work with char32_t, for instance. > > I think the best approach is to make a local matcher for the type predicate. > We want isInteger() unless it's char (that's not explicitly qualified with > signed or unsigned), bool, wchar_t, char16_t, char32_t, or an enumeration > type. > > > Actually, I believe if someone has bool size() in their codebase there's > > still a problem... > > Agreed, though not as part of this check. ;-) Same here, let's wait for a real user asking to support his real container class that defines `uint8_t size() const;`. ================ Comment at: clang-tidy/readability/ContainerSizeEmptyCheck.cpp:145 @@ -144,3 +144,3 @@ } diag(MemberCall->getLocStart(), "the 'empty' method should be used to check " "for emptiness instead of 'size'") ---------------- Since we're expanding the set of supported classes, it makes sense to include the name of the class to the diagnostic message. Repository: rL LLVM https://reviews.llvm.org/D24349 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
