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

[Piotr Padlewski via cfe-commits]

Firstly, please respond in phabricator if it is possible. When you send
email it doesn't appear in phabricator, it's probably a bug.

2016-12-19 8:00 GMT+01:00 Mads Ravn <madsravn at gmail.com>:

> Hi Piotr,
>
> Thank you for your detailed comments :)
>
> I would love some help with the other fixit. I have some notes on it at
> home. But my main catch is that is an implicit cast to boolean from
> str.compare(str) with maybe an ! in front of it. And I need to fix that to
> str.compare(str) == 0 or str.compare(str) != 0.
>
> Why fix it to something, that you will want to fix it again to str == str
and str != str?
I guess you just have to match parent of this expr is negation or anything,
bind negation to some name and then check if you matched to the negation or
not.


> Where should I put the warning in a static const global variable? Is it
> still in StringCompare.cpp or do we have a  joint files for these?
>
> Yep, StringCompare.cpp, just like in other checks.

> Best regards,
> Mads Ravn
>
> On Sun, Dec 18, 2016 at 11:26 PM Piotr Padlewski via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
>> Prazek added a comment.
>>
>> Do you need some help with implementing the other fixit, or you just need
>> some extra time? It seems to be almost the same as your second fixit
>>
>>
>>
>> ================
>> Comment at: clang-tidy/misc/StringCompareCheck.cpp:69-70
>> +    diag(Matched->getLocStart(),
>> +         "do not use 'compare' to test equality of strings; "
>> +         "use the string equality operator instead");
>> +
>> ----------------
>> Take this warning to some static const global variable
>>
>>
>> ================
>> Comment at: clang-tidy/misc/StringCompareCheck.cpp:71
>> +         "use the string equality operator instead");
>> +
>> +  if (const auto *Matched = Result.Nodes.getNodeAs<Stmt>("match2")) {
>> ----------------
>> match1 and match2 are in different matchers (passed to register matchers)?
>>
>> If so put return here after diag to finish control flow for this case.
>>
>>
>> ================
>> Comment at: clang-tidy/misc/StringCompareCheck.cpp:81
>> +      auto Diag = diag(Matched->getLocStart(),
>> +                       "do not use 'compare' to test equality of
>> strings; "
>> +                       "use the string equality operator instead");
>> ----------------
>> and use it here
>>
>>
>> ================
>> Comment at: clang-tidy/misc/StringCompareCheck.h:10-11
>> +
>> +#ifndef STRING_COMPARE_CHECK_H
>> +#define STRING_COMPARE_CHECK_H
>> +
>> ----------------
>> This should be much longer string. Do you know about ./add_new_check?
>>
>> Please make one similar to other checks
>>
>>
>> ================
>> Comment at: clang-tidy/misc/StringCompareCheck.h:36
>> +
>> +#endif // STRING_COMPARE_CHECK_H
>> ----------------
>> DITTO
>>
>>
>> ================
>> Comment at: test/clang-tidy/misc-string-compare.cpp:35-40
>> +  if (str1.compare(str2)) {
>> +  }
>> +  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to
>> test equality of strings; use the string equality operator instead
>> [misc-string-compare]
>> +  if (!str1.compare(str2)) {
>> +  }
>> +  // CHECK-MESSAGES: [[@LINE-2]]:8: warning: do not use 'compare' to
>> test equality of strings; use the string equality operator instead
>> [misc-string-compare]
>> ----------------
>> Why this one doesn't have fixit hint?
>>
>>
>> ================
>> Comment at: test/clang-tidy/misc-string-compare.cpp:70
>> +  }
>> +  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to
>> test equality of strings;
>> +  if (str3->compare(str2) == 0) {
>> ----------------
>> no fixit?
>>
>>
>> https://reviews.llvm.org/D27210
>>
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20161219/a8899d36/attachment.html>