[PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Mads Ravn via Phabricator via cfe-commits]

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