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

Mads Ravn via cfe-commits cfe-commits at lists.llvm.org
Sun Dec 18 23:00:16 PST 2016


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.

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?

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/56355f4a/attachment.html>


More information about the cfe-commits mailing list