[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
Mon Dec 26 11:50:54 PST 2016


Hi,

The last mail was only meant to contain the question about the comment to
misc-suspicious-string-compare check.

Do you reckon I should remove that match from my check? Or should we move
it?

Best regards,
Mads Ravn

On Mon, Dec 26, 2016 at 8:48 PM Mads Ravn via Phabricator <
reviews at reviews.llvm.org> wrote:

> 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20161226/7e9264bb/attachment.html>


More information about the cfe-commits mailing list