aaron.ballman added a comment. In https://reviews.llvm.org/D24349#538098, @alexfh wrote:
> 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. That means there is no way to silence this check on false positives aside from disabling it entirely, which is not a particularly good user experience. However, I do agree that if we constrain the heuristic appropriately, the number of false positives should be low. ================ Comment at: clang-tidy/readability/ContainerSizeEmptyCheck.cpp:33 @@ +32,3 @@ + const auto validContainer = namedDecl( + has(functionDecl( + isPublic(), hasName("size"), returns(isInteger()), ---------------- alexfh wrote: > 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;`. I'm not certain I agree with that (I don't like adding new functionality that is known to be imprecise, especially when dealing with heuristic-based checking), but I don't have that strong of feelings. 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
