<div dir="ltr">Hi Piotr,<div><br></div><div>That is a good point. Because it is not always -1 or 1 that determines lexicographical higher or lower. </div><div><br></div><div>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? </div><div><br></div><div>Best regards,</div><div>Mads Ravn</div></div><br><div class="gmail_quote"><div dir="ltr">On Thu, Dec 15, 2016 at 8:17 PM Piotr Padlewski via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Prazek added a comment.<br class="gmail_msg">
<br class="gmail_msg">
Good job.<br class="gmail_msg">
I think it is resonable to warn in cases like:<br class="gmail_msg">
<br class="gmail_msg">
  if (str.compare(str2) == 1)<br class="gmail_msg">
<br class="gmail_msg">
or even<br class="gmail_msg">
<br class="gmail_msg">
  if(str.compare(str2) == -1)<br class="gmail_msg">
<br class="gmail_msg">
Sometimes people check for 1 or -1 instead of > 0 and < 0. If I remember corectly PVS studio found some bugs like this.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: clang-tidy/misc/StringCompareCheck.cpp:27<br class="gmail_msg">
+                               hasName("::std::basic_string"))))),<br class="gmail_msg">
+      hasArgument(0, declRefExpr()), callee(memberExpr()));<br class="gmail_msg">
+<br class="gmail_msg">
----------------<br class="gmail_msg">
malcolm.parsons wrote:<br class="gmail_msg">
> I don't think you need to check what the first argument is.<br class="gmail_msg">
+1, just check if you are calling function with 1 argument.<br class="gmail_msg">
you can still use hasArgument(0, expr().bind("str2")) to bind argument<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: clang-tidy/misc/StringCompareCheck.cpp:25<br class="gmail_msg">
+    return;<br class="gmail_msg">
+  const auto strCompare = cxxMemberCallExpr(<br class="gmail_msg">
+      callee(cxxMethodDecl(hasName("compare"),<br class="gmail_msg">
----------------<br class="gmail_msg">
Start with upper case<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<a href="https://reviews.llvm.org/D27210" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D27210</a><br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
</blockquote></div>