alexfh requested changes to this revision. alexfh added a comment. This revision now requires changes to proceed.
In https://reviews.llvm.org/D24349#538102, @aaron.ballman wrote: > 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. Just checked: readability-container-size-empty with the patch finds a few thousand new warnings related to about 400 unique container names in our code. A representative sample shows no false positives and we've found just a single false negative (that triggers the check without the patch) - a container's `size()` method was not `const`. Which makes me pretty sure that: 1. The new algorithm is much more consistent and useful than the whitelist approach (gathering and maintaining a whitelist of a few hundred names is also not fun at all). 2. Given the size of our code and the number of third-party libraries, possibility of false positives is extremely low. 3. If there are reports of false positives, we could go with a blacklist approach instead, but I don't think this will ever be needed. ================ Comment at: clang-tidy/readability/ContainerSizeEmptyCheck.cpp:33 @@ +32,3 @@ + const auto validContainer = cxxRecordDecl(isSameOrDerivedFrom( + namedDecl(has(cxxMethodDecl(isConst(), parameterCountIs(0), isPublic(), + hasName("size"), returns(isInteger()), ---------------- I don't think we can come up with a proper solution until we see a real world example. If we want to find one faster, we can allow all character types and wait for reports of false positives ;) ================ Comment at: docs/clang-tidy/checks/readability-container-size-empty.rst:18 @@ +17,3 @@ + +Check issues warning if a container has `size()` and `empty()` methods matching +following signatures: ---------------- `The check` ================ Comment at: docs/clang-tidy/checks/readability-container-size-empty.rst:18 @@ +17,3 @@ + +Check issues warning if a container has `size()` and `empty()` methods matching +following signatures: ---------------- alexfh wrote: > `The check` Double backquotes should be used. ================ Comment at: docs/clang-tidy/checks/readability-container-size-empty.rst:21 @@ +20,3 @@ + +code-block::c++ + ---------------- nit: space before `c++`. ================ Comment at: docs/clang-tidy/checks/readability-container-size-empty.rst:23 @@ +22,3 @@ + + size_type size() const; + bool empty() const; ---------------- Explain the constraints `size_type` should satisfy. https://reviews.llvm.org/D24349 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
