<div dir="ltr">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</div><div class="gmail_extra"><br><div class="gmail_quote">2016-12-15 20:55 GMT+01:00 Mads Ravn <span dir="ltr"><<a href="mailto:madsravn@gmail.com" target="_blank">madsravn@gmail.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><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><div class="HOEnZb"><div class="h5"><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" target="_blank">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="m_1742080108376063199gmail_msg">
<br class="m_1742080108376063199gmail_msg">
Good job.<br class="m_1742080108376063199gmail_msg">
I think it is resonable to warn in cases like:<br class="m_1742080108376063199gmail_msg">
<br class="m_1742080108376063199gmail_msg">
if (str.compare(str2) == 1)<br class="m_1742080108376063199gmail_msg">
<br class="m_1742080108376063199gmail_msg">
or even<br class="m_1742080108376063199gmail_msg">
<br class="m_1742080108376063199gmail_msg">
if(str.compare(str2) == -1)<br class="m_1742080108376063199gmail_msg">
<br class="m_1742080108376063199gmail_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="m_1742080108376063199gmail_msg">
<br class="m_1742080108376063199gmail_msg">
<br class="m_1742080108376063199gmail_msg">
<br class="m_1742080108376063199gmail_msg">
================<br class="m_1742080108376063199gmail_msg">
Comment at: clang-tidy/misc/<wbr>StringCompareCheck.cpp:27<br class="m_1742080108376063199gmail_msg">
+ hasName("::std::basic_string"<wbr>))))),<br class="m_1742080108376063199gmail_msg">
+ hasArgument(0, declRefExpr()), callee(memberExpr()));<br class="m_1742080108376063199gmail_msg">
+<br class="m_1742080108376063199gmail_msg">
----------------<br class="m_1742080108376063199gmail_msg">
malcolm.parsons wrote:<br class="m_1742080108376063199gmail_msg">
> I don't think you need to check what the first argument is.<br class="m_1742080108376063199gmail_msg">
+1, just check if you are calling function with 1 argument.<br class="m_1742080108376063199gmail_msg">
you can still use hasArgument(0, expr().bind("str2")) to bind argument<br class="m_1742080108376063199gmail_msg">
<br class="m_1742080108376063199gmail_msg">
<br class="m_1742080108376063199gmail_msg">
================<br class="m_1742080108376063199gmail_msg">
Comment at: clang-tidy/misc/<wbr>StringCompareCheck.cpp:25<br class="m_1742080108376063199gmail_msg">
+ return;<br class="m_1742080108376063199gmail_msg">
+ const auto strCompare = cxxMemberCallExpr(<br class="m_1742080108376063199gmail_msg">
+ callee(cxxMethodDecl(hasName("<wbr>compare"),<br class="m_1742080108376063199gmail_msg">
----------------<br class="m_1742080108376063199gmail_msg">
Start with upper case<br class="m_1742080108376063199gmail_msg">
<br class="m_1742080108376063199gmail_msg">
<br class="m_1742080108376063199gmail_msg">
<a href="https://reviews.llvm.org/D27210" rel="noreferrer" class="m_1742080108376063199gmail_msg" target="_blank">https://reviews.llvm.org/<wbr>D27210</a><br class="m_1742080108376063199gmail_msg">
<br class="m_1742080108376063199gmail_msg">
<br class="m_1742080108376063199gmail_msg">
<br class="m_1742080108376063199gmail_msg">
</blockquote></div>
</div></div></blockquote></div><br></div>