I think that the feature I mentioned is right thing to put in this check, however you don't have to implement it right now, just leave FIXIT comment
2016-12-15 20:55 GMT+01:00 Mads Ravn <madsr...@gmail.com>: > Hi Piotr, > > That is a good point. Because it is not always -1 or 1 that determines > lexicographical higher or lower. > > However, I don't think that is in the scope of this check. This check > checks for string comparison (equality or inequality). Adding a match for > if the user is using the compare function semantically wrong might make the > check too ambiguous. Since str.compare(str) == 0 will check for equality > and str.compare(str) != 0 will check for inequality. But str.compare(str) > == 1 will check whether one string is lexicographically smaller than the > other (and == -1 also). What do you think? > > Best regards, > Mads Ravn > > On Thu, Dec 15, 2016 at 8:17 PM Piotr Padlewski via Phabricator < > revi...@reviews.llvm.org> wrote: > >> Prazek added a comment. >> >> Good job. >> I think it is resonable to warn in cases like: >> >> if (str.compare(str2) == 1) >> >> or even >> >> if(str.compare(str2) == -1) >> >> Sometimes people check for 1 or -1 instead of > 0 and < 0. If I remember >> corectly PVS studio found some bugs like this. >> >> >> >> ================ >> Comment at: clang-tidy/misc/StringCompareCheck.cpp:27 >> + hasName("::std::basic_string"))))), >> + hasArgument(0, declRefExpr()), callee(memberExpr())); >> + >> ---------------- >> malcolm.parsons wrote: >> > I don't think you need to check what the first argument is. >> +1, just check if you are calling function with 1 argument. >> you can still use hasArgument(0, expr().bind("str2")) to bind argument >> >> >> ================ >> Comment at: clang-tidy/misc/StringCompareCheck.cpp:25 >> + return; >> + const auto strCompare = cxxMemberCallExpr( >> + callee(cxxMethodDecl(hasName("compare"), >> ---------------- >> Start with upper case >> >> >> https://reviews.llvm.org/D27210 >> >> >> >>
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits