Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy
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 <
revi...@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
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy
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 :
> 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 <
> revi...@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("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
>>
>>
>>
>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy
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 <
revi...@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("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
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy
I think that the feature I mentioned is right thing to put in this check,
however you don't have to implement it right now, just leave FIXIT comment
2016-12-15 20:55 GMT+01:00 Mads Ravn :
> Hi Piotr,
>
> That is a good point. Because it is not always -1 or 1 that determines
> lexicographical higher or lower.
>
> However, I don't think that is in the scope of this check. This check
> checks for string comparison (equality or inequality). Adding a match for
> if the user is using the compare function semantically wrong might make the
> check too ambiguous. Since str.compare(str) == 0 will check for equality
> and str.compare(str) != 0 will check for inequality. But str.compare(str)
> == 1 will check whether one string is lexicographically smaller than the
> other (and == -1 also). What do you think?
>
> Best regards,
> Mads Ravn
>
> On Thu, Dec 15, 2016 at 8:17 PM Piotr Padlewski via Phabricator <
> revi...@reviews.llvm.org> wrote:
>
>> Prazek added a comment.
>>
>> Good job.
>> I think it is resonable to warn in cases like:
>>
>> if (str.compare(str2) == 1)
>>
>> or even
>>
>> if(str.compare(str2) == -1)
>>
>> Sometimes people check for 1 or -1 instead of > 0 and < 0. If I remember
>> corectly PVS studio found some bugs like this.
>>
>>
>>
>>
>> Comment at: clang-tidy/misc/StringCompareCheck.cpp:27
>> + hasName("::std::basic_string"),
>> + hasArgument(0, declRefExpr()), callee(memberExpr()));
>> +
>>
>> malcolm.parsons wrote:
>> > I don't think you need to check what the first argument is.
>> +1, just check if you are calling function with 1 argument.
>> you can still use hasArgument(0, expr().bind("str2")) to bind argument
>>
>>
>>
>> Comment at: clang-tidy/misc/StringCompareCheck.cpp:25
>> +return;
>> + const auto strCompare = cxxMemberCallExpr(
>> + callee(cxxMethodDecl(hasName("compare"),
>>
>> Start with upper case
>>
>>
>> https://reviews.llvm.org/D27210
>>
>>
>>
>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy
Hi Piotr,
That is a good point. Because it is not always -1 or 1 that determines
lexicographical higher or lower.
However, I don't think that is in the scope of this check. This check
checks for string comparison (equality or inequality). Adding a match for
if the user is using the compare function semantically wrong might make the
check too ambiguous. Since str.compare(str) == 0 will check for equality
and str.compare(str) != 0 will check for inequality. But str.compare(str)
== 1 will check whether one string is lexicographically smaller than the
other (and == -1 also). What do you think?
Best regards,
Mads Ravn
On Thu, Dec 15, 2016 at 8:17 PM Piotr Padlewski via Phabricator <
revi...@reviews.llvm.org> wrote:
> Prazek added a comment.
>
> Good job.
> I think it is resonable to warn in cases like:
>
> if (str.compare(str2) == 1)
>
> or even
>
> if(str.compare(str2) == -1)
>
> Sometimes people check for 1 or -1 instead of > 0 and < 0. If I remember
> corectly PVS studio found some bugs like this.
>
>
>
>
> Comment at: clang-tidy/misc/StringCompareCheck.cpp:27
> + hasName("::std::basic_string"),
> + hasArgument(0, declRefExpr()), callee(memberExpr()));
> +
>
> malcolm.parsons wrote:
> > I don't think you need to check what the first argument is.
> +1, just check if you are calling function with 1 argument.
> you can still use hasArgument(0, expr().bind("str2")) to bind argument
>
>
>
> Comment at: clang-tidy/misc/StringCompareCheck.cpp:25
> +return;
> + const auto strCompare = cxxMemberCallExpr(
> + callee(cxxMethodDecl(hasName("compare"),
>
> Start with upper case
>
>
> https://reviews.llvm.org/D27210
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy
Hi Malcolm,
I will look into fixing the two cases only.
argumentCountIs(1) is sufficient to narrow the matching to only string
compare with one argument.
Best regards,
Mads Ravn
On Mon, Dec 12, 2016 at 10:38 AM Malcolm Parsons via Phabricator <
revi...@reviews.llvm.org> wrote:
> malcolm.parsons added inline comments.
>
>
>
> Comment at: clang-tidy/misc/StringCompareCheck.cpp:25
> + callee(cxxMethodDecl(hasName("compare"),
> + ofClass(classTemplateSpecializationDecl(
> + hasName("::std::basic_string"),
>
> malcolm.parsons wrote:
> > malcolm.parsons wrote:
> > > This needs to check that it's one of the single parameter overloads of
> compare.
> > Add `parameterCountIs(1)`.
> Actually, the `argumentCountIs(1)` below should be sufficient.
>
>
> https://reviews.llvm.org/D27210
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy
Hi guys,
Do you have any extra comments for this?
Best regards
On Sat, Dec 3, 2016 at 1:34 PM Mads Ravn via Phabricator <
revi...@reviews.llvm.org> wrote:
> madsravn updated this revision to Diff 80177.
> madsravn added a comment.
>
> Did as comments suggested: Fixed the description about compare returning
> -1, 0 or 1. Fixed the ast matcher to only find compare with one argument.
> Clang-formatted everything. Added a new test (str.compare("foo")) and wrote
> a FIXME for the fixit.
>
>
> https://reviews.llvm.org/D27210
>
> Files:
> clang-tidy/misc/CMakeLists.txt
> clang-tidy/misc/MiscTidyModule.cpp
> clang-tidy/misc/StringCompareCheck.cpp
> clang-tidy/misc/StringCompareCheck.h
> docs/ReleaseNotes.rst
> docs/clang-tidy/checks/list.rst
> docs/clang-tidy/checks/misc-string-compare.rst
> test/clang-tidy/misc-string-compare.cpp
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy
On 2 December 2016 at 10:29, Mads Ravn wrote: > alexfh suggested that fixits seemed easy to implement. I am having a few > doubts as to how I would make fixits for case 1 & 2. How important would it > be to implement fixits at this point? Add a FIXME comment if they're difficult. -- Malcolm Parsons ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy
Hi Malcolm, Thanks. I will fix the last couple of things in the weekend and hopefully have something worth showing there. alexfh suggested that fixits seemed easy to implement. I am having a few doubts as to how I would make fixits for case 1 & 2. How important would it be to implement fixits at this point? Best regards, Mads Ravn On Fri, Dec 2, 2016 at 10:57 AM Malcolm Parsons wrote: > On 2 December 2016 at 09:50, Mads Ravn wrote: > >> Comment at: test/clang-tidy/misc-string-compare.cpp:9 > >> + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test > >> equality of strings; use the string equality operator instead > >> [misc-string-compare] > > What do you mean by this comment? > > I was replying to my previous line comment, but the line that was > commented on has changed since. > > My comment was: > > There's still no test for the single parameter c-string overload. > > -- > Malcolm Parsons > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy
On 2 December 2016 at 09:50, Mads Ravn wrote: >> Comment at: test/clang-tidy/misc-string-compare.cpp:9 >> + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test >> equality of strings; use the string equality operator instead >> [misc-string-compare] > What do you mean by this comment? I was replying to my previous line comment, but the line that was commented on has changed since. My comment was: There's still no test for the single parameter c-string overload. -- Malcolm Parsons ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy
Hi Malcolm,
Matching for the single parameter overload of compare is probably a good
idea. I will do that.
> Comment at: test/clang-tidy/misc-string-compare.cpp:9
> + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test
equality of strings; use the string equality operator instead
[misc-string-compare]
What do you mean by this comment?
Best regards,
Mads Ravn
On Fri, Dec 2, 2016 at 10:26 AM Malcolm Parsons via Phabricator <
revi...@reviews.llvm.org> wrote:
> malcolm.parsons added inline comments.
>
>
>
> Comment at: clang-tidy/misc/StringCompareCheck.cpp:25
> + callee(cxxMethodDecl(hasName("compare"),
> + ofClass(classTemplateSpecializationDecl(
> + hasName("::std::basic_string"),
>
> This needs to check that it's one of the single parameter overloads of
> compare.
>
>
>
> Comment at: clang-tidy/misc/StringCompareCheck.cpp:27
> + hasName("::std::basic_string"),
> + hasArgument(0, declRefExpr()), callee(memberExpr()));
> +
>
> I don't think you need to check what the first argument is.
>
>
>
> Comment at: clang-tidy/misc/StringCompareCheck.cpp:39
> + binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!=")),
> + hasEitherOperand(strCompare),
> +
> hasEitherOperand(integerLiteral(equals(0
>
> Is this clang-formatted?
>
>
>
> 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) {}
> > ```
> There's still no test for the single parameter c-string overload.
>
>
> https://reviews.llvm.org/D27210
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy
On 1 Dec 2016 8:37 p.m., "Mads Ravn" wrote:
> I see the idea for the fixit clearly for case 3 & 4. Just erase
.compare(str2) and replace 0 with str2. I have a quick question though:
Given the declRefExpr().bind("str2"), how do I read the name of it in
clang-tidy? Or should I just bind 0 as well and then create replacement
with str where const auto str = Result.Nodes.getNodeAs("str2") ?
You could use
FixItHint::CreateInsertionFromRange to copy str2 to 0, or leave str2 where
it is, remove the binary operator and create a new one between the strings.
> Where I seem to find a little trouble is how to fixit case 1 & 2 now that
they are reduced to one case. How do I check whether or not there is a
unary operator in front of the implicitCast?
No idea.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy
Hi Malcolm,
Thanks for the suggestions, I have been reading up on the fixits.
My initial four cases has been reduced to two a little more general cases:
1 & 2: implicitCast to bool str1.compare(str2). This case covers both
!str1.compare(str2) and str1.compare(str2)
3 & 4: str1.compare(str2) == 0 and str1.compare(str2) != 0.
I see the idea for the fixit clearly for case 3 & 4. Just erase
.compare(str2) and replace 0 with str2. I have a quick question though:
Given the declRefExpr().bind("str2"), how do I read the name of it in
clang-tidy? Or should I just bind 0 as well and then create replacement
with str where const auto str = Result.Nodes.getNodeAs("str2") ?
Where I seem to find a little trouble is how to fixit case 1 & 2 now that
they are reduced to one case. How do I check whether or not there is a
unary operator in front of the implicitCast?
Thank you,
Mads Ravn
On Thu, Dec 1, 2016 at 8:53 PM Mads Ravn via Phabricator <
revi...@reviews.llvm.org> wrote:
> madsravn updated this revision to Diff 79961.
> madsravn added a comment.
>
> Fixed broken tests.
>
>
> https://reviews.llvm.org/D27210
>
> Files:
> clang-tidy/misc/CMakeLists.txt
> clang-tidy/misc/MiscTidyModule.cpp
> clang-tidy/misc/StringCompareCheck.cpp
> clang-tidy/misc/StringCompareCheck.h
> docs/ReleaseNotes.rst
> docs/clang-tidy/checks/list.rst
> docs/clang-tidy/checks/misc-string-compare.rst
> test/clang-tidy/misc-string-compare.cpp
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy
On 1 December 2016 at 16:42, Mads Ravn wrote:
> I have now implemented your suggestions - all but the fixit one. If I have
> added bindings for str1 and str2 in ast matcher, how would I go about
> creating a replacement for the entire implicitCastExpr or binaryOperator? I
> can't find any example in the code for clang-tidy to suggest how I would
> build a new node for the AST. Or I am looking at it from a wrong direction?
You create insertions, removals and replacements that affect the source code.
The AST is not changed.
Fix-it hints are documented here:
http://clang.llvm.org/docs/InternalsManual.html#fix-it-hints
Source locations and ranges are documented here:
http://clang.llvm.org/docs/InternalsManual.html#sourcelocation
To turn str1.compare(str2) into str1 == str2 you need to replace
".compare(" with " == " and remove ")".
You can get the SourceLocation of the . by calling getOperatorLoc() on
the MemberExpr.
You can tell if you have a . or an -> by calling isArrow() on the MemberExpr.
You can get the SourceLocation of the ) by calling getRParenLoc() on
the CXXMemberCallExpr.
--
Malcolm Parsons
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy
Hi Alexander,
I have now implemented your suggestions - all but the fixit one. If I have
added bindings for str1 and str2 in ast matcher, how would I go about
creating a replacement for the entire implicitCastExpr or binaryOperator? I
can't find any example in the code for clang-tidy to suggest how I would
build a new node for the AST. Or I am looking at it from a wrong direction?
Could you hint me in the right direction as to how I would make the fixit?
Best regards,
Mads Ravn
On Thu, Dec 1, 2016 at 3:41 PM Alexander Kornienko via Phabricator <
revi...@reviews.llvm.org> wrote:
> alexfh requested changes to this revision.
> alexfh added inline comments.
> This revision now requires changes to proceed.
>
>
>
> Comment at: clang-tidy/misc/StringCompareCheck.cpp:29
> +
> + // First and second case: cast str.compare(str) to boolean
> + Finder->addMatcher(
>
> Please add trailing periods here and elsewhere.
>
>
>
> Comment at: clang-tidy/misc/StringCompareCheck.cpp:36-50
> + // Third case: str.compare(str) == 0
> + Finder->addMatcher(
> + binaryOperator(hasOperatorName("=="),
> + hasEitherOperand(strCompare),
> +
> hasEitherOperand(integerLiteral(equals(0
> + .bind("match"),
> + this);
>
> These two matchers can be merged to avoid repetition.
>
>
>
> Comment at: clang-tidy/misc/StringCompareCheck.cpp:55-57
> +diag(Matched->getLocStart(),
> + "do not use compare to test equality of strings; "
> + "use the string equality operator instead");
>
> It looks like it's relatively straightforward to implement fixit hints.
> WDYT?
>
>
>
> Comment at: test/clang-tidy/misc-string-compare.cpp:29
> + if(!str1.compare(str2)) {}
> + // CHECK-MESSAGES: [[@LINE-1]]:7: warning: do not use compare to test
> equality of strings; use the string equality operator instead
> [misc-string-compare]
> + if(str1.compare(str2) == 0) {}
>
> Truncate all CHECK patterns after the first one after `of strings;`
>
>
> https://reviews.llvm.org/D27210
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy
I think I got it. I will throw a new diff up within the hour. Thanks for the ideas :) On Wed, Nov 30, 2016 at 6:48 PM Malcolm Parsons wrote: > On 30 November 2016 at 17:18, Mads Ravn wrote: > > So remove the ifStmt from the third and fourth case? > > I was thinking all cases. > Can the first case be restricted to casts to bool? > If not, keep the cast to int case with an ifStmt and add a cast to bool > case. > Does it matter whether the cast is implicit? > > -- > Malcolm Parsons > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy
On 30 November 2016 at 17:18, Mads Ravn wrote: > So remove the ifStmt from the third and fourth case? I was thinking all cases. Can the first case be restricted to casts to bool? If not, keep the cast to int case with an ifStmt and add a cast to bool case. Does it matter whether the cast is implicit? -- Malcolm Parsons ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy
So remove the ifStmt from the third and fourth case? So that I keep if(str1.compare(str2)) and if(!str1.compare(str2)), and change the other two to str1.compare(str2) == 0 and str1.compare(str2) != 0 ? That makes good sense. Then I could also add some of the test cases you mentioned earlier. On Wed, Nov 30, 2016 at 5:59 PM Malcolm Parsons via Phabricator < revi...@reviews.llvm.org> wrote: > malcolm.parsons added a comment. > > I don't know why you're restricting this check to only match within the > condition of an if statement. > > > https://reviews.llvm.org/D27210 > > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
