madsravn marked 2 inline comments as done. madsravn added inline comments.
================ Comment at: clang-tidy/misc/MiscStringCompareCheck.h:24 +/// http://clang.llvm.org/extra/clang-tidy/checks/misc-string-compare-check.html +class MiscStringCompareCheck : public ClangTidyCheck { +public: ---------------- malcolm.parsons wrote: > Remove `Misc`. > > Did you use add_new_check.py to add this check? No, but the files I was looking at had the same naming convention. Maybe something has changed in that regards recently? I will fix it. ================ Comment at: clang-tidy/misc/StringCompareCheck.cpp:25 + "operator instead"; +static const StringRef GuaranteeMessage = "'compare' is not guaranteed to " + "return -1 or 1; check for bigger or " ---------------- malcolm.parsons wrote: > misc-suspicious-string-compare warns about suspicious `strcmp()`; maybe it > should handle `string::compare()` too. Do you suggest that I move this check to misc-suspicious-string-compare? Or should we just remove it from here? ================ Comment at: docs/clang-tidy/checks/misc-string-compare.rst:10 +equality or inequality operators. The compare method is intended for sorting +functions and thus returns ``-1``, ``0`` or ``1`` depending on the lexicographical +relationship between the strings compared. If an equality or inequality check ---------------- xazax.hun wrote: > As far as I remember this is not true. The ``compare`` method can return any > integer number, only the sign is defined. It is not guaranteed to return -1 > or 1 in case of inequality. This is true. I checked it - it is just some implementations which tend to use -1, 0 and 1. However, the specification says negative, 0 and positive. I will correct it. Thanks ================ Comment at: test/clang-tidy/misc-string-compare.cpp:9 + + if(str1.compare(str2)) {} + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare] ---------------- malcolm.parsons wrote: > Some other test ideas: > > ``` > if (str1.compare("foo")) {} > > return str1.compare(str2) == 0; > > func(str1.compare(str2) != 0); > > if (str2.empty() || str1.compare(str2) != 0) {} > ``` None of those fit the ast match. I think it's fine as it is now. If the matcher will be expanded to check for some of those cases, I think more test cases are needed. https://reviews.llvm.org/D27210 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
