<div dir="ltr">Hi,<div><br></div><div>The last mail was only meant to contain the question about the comment to misc-suspicious-string-compare check.</div><div><br></div><div>Do you reckon I should remove that match from my check? Or should we move it? </div><div><br></div><div>Best regards,</div><div>Mads Ravn</div></div><br><div class="gmail_quote"><div dir="ltr">On Mon, Dec 26, 2016 at 8:48 PM Mads Ravn 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">madsravn marked 2 inline comments as done.<br class="gmail_msg">
madsravn added inline comments.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: clang-tidy/misc/MiscStringCompareCheck.h:24<br class="gmail_msg">
+/// <a href="http://clang.llvm.org/extra/clang-tidy/checks/misc-string-compare-check.html" rel="noreferrer" class="gmail_msg" target="_blank">http://clang.llvm.org/extra/clang-tidy/checks/misc-string-compare-check.html</a><br class="gmail_msg">
+class MiscStringCompareCheck : public ClangTidyCheck {<br class="gmail_msg">
+public:<br class="gmail_msg">
----------------<br class="gmail_msg">
malcolm.parsons wrote:<br class="gmail_msg">
> Remove `Misc`.<br class="gmail_msg">
><br class="gmail_msg">
> Did you use add_new_check.py to add this check?<br class="gmail_msg">
No, but the files I was looking at had the same naming convention. Maybe something has changed in that regards recently?<br class="gmail_msg">
<br class="gmail_msg">
I will fix it.<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">
+ "operator instead";<br class="gmail_msg">
+static const StringRef GuaranteeMessage = "'compare' is not guaranteed to "<br class="gmail_msg">
+ "return -1 or 1; check for bigger or "<br class="gmail_msg">
----------------<br class="gmail_msg">
malcolm.parsons wrote:<br class="gmail_msg">
> misc-suspicious-string-compare warns about suspicious `strcmp()`; maybe it should handle `string::compare()` too.<br class="gmail_msg">
Do you suggest that I move this check to misc-suspicious-string-compare? Or should we just remove it from here?<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: docs/clang-tidy/checks/misc-string-compare.rst:10<br class="gmail_msg">
+equality or inequality operators. The compare method is intended for sorting<br class="gmail_msg">
+functions and thus returns ``-1``, ``0`` or ``1`` depending on the lexicographical<br class="gmail_msg">
+relationship between the strings compared. If an equality or inequality check<br class="gmail_msg">
----------------<br class="gmail_msg">
xazax.hun wrote:<br class="gmail_msg">
> 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.<br class="gmail_msg">
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<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: test/clang-tidy/misc-string-compare.cpp:9<br class="gmail_msg">
+<br class="gmail_msg">
+ if(str1.compare(str2)) {}<br class="gmail_msg">
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]<br class="gmail_msg">
----------------<br class="gmail_msg">
malcolm.parsons wrote:<br class="gmail_msg">
> Some other test ideas:<br class="gmail_msg">
><br class="gmail_msg">
> ```<br class="gmail_msg">
> if (str1.compare("foo")) {}<br class="gmail_msg">
><br class="gmail_msg">
> return str1.compare(str2) == 0;<br class="gmail_msg">
><br class="gmail_msg">
> func(str1.compare(str2) != 0);<br class="gmail_msg">
><br class="gmail_msg">
> if (str2.empty() || str1.compare(str2) != 0) {}<br class="gmail_msg">
> ```<br class="gmail_msg">
None of those fit the ast match.<br class="gmail_msg">
<br class="gmail_msg">
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.<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>