Hi, The last mail was only meant to contain the question about the comment to misc-suspicious-string-compare check.
Do you reckon I should remove that match from my check? Or should we move it? Best regards, Mads Ravn On Mon, Dec 26, 2016 at 8:48 PM Mads Ravn via Phabricator < revi...@reviews.llvm.org> wrote: > 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
