[PATCH] D143971: [clang-tidy] Flag more buggy string constructor cases
PiotrZSL added inline comments.
Comment at: clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp:69
void StringConstructorCheck::registerMatchers(MatchFinder *Finder) {
+ const auto SignedCharType = qualType(isAnyCharacter(), isSignedInteger());
const auto ZeroExpr = expr(ignoringParenImpCasts(integerLiteral(equals(0;
this may incorrectly work also for signed char, we want it work only for char
and wchat_t, not signed char, unsigned char, signed wchar_t, unsigned wchar_t.
So this still need some work. I would say new "isAnyCharacter" need to be
created.
Comment at: clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp:96
+ const auto NonCharacterInteger =
+ qualType(isInteger(), unless(isAnyCharacter()));
+ const auto CharToIntCastExpr = implicitCastExpr(
Similar could be here, we consider std::uint8_t and std::sint8_t as integers,
not as an characters, same with signed char, unsigned char
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143971/new/
https://reviews.llvm.org/D143971
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D143971: [clang-tidy] Flag more buggy string constructor cases
ccotter added a comment. Thanks, fixed the first false positive example you gave. Let me think about the second example in your most recent post: char c = '\n'; using Size = int; Size size = 10U; std::string str2(c, size); This is my newly added case `swapped4`, where one of my original intents was to flag this code as "swapped." I can see the the ambiguity. Worst case, "confusing string fill constructor arguments" is the fallback that I think is acceptable, and still achieves the goal of at least flagging the code as potentially buggy. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143971/new/ https://reviews.llvm.org/D143971 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D143971: [clang-tidy] Flag more buggy string constructor cases
ccotter updated this revision to Diff 530349.
ccotter added a comment.
- Exclude false positive
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143971/new/
https://reviews.llvm.org/D143971
Files:
clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/bugprone/string-constructor.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone/string-constructor.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/string-constructor.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/string-constructor.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/string-constructor.cpp
@@ -5,12 +5,13 @@
class allocator {};
template
class char_traits {};
-template , typename A = std::allocator >
+template , typename A = std::allocator>
struct basic_string {
basic_string();
- basic_string(const C*, unsigned int size);
- basic_string(const C *, const A &allocator = A());
- basic_string(unsigned int size, C c);
+ basic_string(const basic_string&, unsigned int, unsigned int, const A & = A());
+ basic_string(const C*, unsigned int);
+ basic_string(const C*, const A& = A());
+ basic_string(unsigned int, C);
};
typedef basic_string string;
typedef basic_string wstring;
@@ -18,7 +19,7 @@
template >
struct basic_string_view {
basic_string_view();
- basic_string_view(const C *, unsigned int size);
+ basic_string_view(const C *, unsigned int);
basic_string_view(const C *);
};
typedef basic_string_view string_view;
@@ -28,20 +29,76 @@
const char* kText = "";
const char kText2[] = "";
extern const char kText3[];
+char getChar();
+const char* getCharPtr();
+
+struct CharLikeObj {
+ operator char() const;
+};
+
+unsigned char getUChar();
void Test() {
- std::string str('x', 4);
- // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor parameters are probably swapped; expecting string(count, character) [bugprone-string-constructor]
- // CHECK-FIXES: std::string str(4, 'x');
- std::wstring wstr(L'x', 4);
- // CHECK-MESSAGES: [[@LINE-1]]:16: warning: string constructor parameters are probably swapped
- // CHECK-FIXES: std::wstring wstr(4, L'x');
+ short sh;
+ int i;
+ int& ref_i = i;
+ char ch;
+ CharLikeObj char_like_obj;
+
+ std::string swapped('x', 4);
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped(4, 'x');
+ std::wstring wswapped(L'x', 4);
+ // CHECK-MESSAGES: [[@LINE-1]]:16: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::wstring wswapped(4, L'x');
+ std::string swapped2('x', i);
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped2(i, 'x');
+ std::string swapped3(ch, 4);
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped3(4, ch);
+ std::string swapped4(ch, i);
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped4(i, ch);
+ std::string swapped5('x', (int)'x');
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped5((int)'x', 'x');
+ std::string swapped6(getChar(), 10);
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped6(10, getChar());
+ std::string swapped7((('x')), ( i ));
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped7(( i ), (('x')));
+ std::string swapped8((ch), (i));
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped8((i), (ch));
+ std::string swapped9((getChar()), (i));
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped9((i), (getChar()));
std::string s0(0, 'x');
// CHECK-MESSAGES: [[@LINE-1]
[PATCH] D143971: [clang-tidy] Flag more buggy string constructor cases
PiotrZSL requested changes to this revision. PiotrZSL added a comment. This revision now requires changes to proceed. I run into false positive with this example: using UInt8 = unsigned char; UInt8 size = 5U; std::string str2(size, 'x'); // warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor] This is due to change in CharExpr, simply isAnyCharacter is too wide, as it also match signed/unsigned characters, and your tests does not cover those. Probably for this we need to be more strict, and accept only non signed/unsigned characters. Other issue is with this: char c = '\n'; using Size = int; Size size = 10U; std::string str2(c, size); Even that in AST we got double implicit casts, it's detected as `warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]`. Instead of being detected as 'confusing string fill constructor arguments'. This probably could be fixed by matching those double casts first, instead of last. | `-VarDecl 0x56436ceb0520 col:15 str2 'std::string':'std::basic_string' callinit | `-CXXConstructExpr 0x56436ceb0650 'std::string':'std::basic_string' 'void (unsigned int, char)' | |-ImplicitCastExpr 0x56436ceb0608 'unsigned int' | | `-ImplicitCastExpr 0x56436ceb05f0 'char' | | `-DeclRefExpr 0x56436ceb0588 'char' lvalue Var 0x56436ceb0280 'c' 'char' | `-ImplicitCastExpr 0x56436ceb0638 'char':'char' | `-ImplicitCastExpr 0x56436ceb0620 'Size':'int' | `-DeclRefExpr 0x56436ceb05a8 'Size':'int' lvalue Var 0x56436ceb0420 'size' 'Size':'int' Except above 2 change looks OK to me. My main concern is false-positive, simply because in project that I work for we use lot of std::uint8_t as size, to save bytes in struct. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143971/new/ https://reviews.llvm.org/D143971 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D143971: [clang-tidy] Flag more buggy string constructor cases
ccotter added a comment. bump please? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143971/new/ https://reviews.llvm.org/D143971 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D143971: [clang-tidy] Flag more buggy string constructor cases
ccotter added a comment. bump please? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143971/new/ https://reviews.llvm.org/D143971 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D143971: [clang-tidy] Flag more buggy string constructor cases
ccotter updated this revision to Diff 520143.
ccotter added a comment.
rebase
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143971/new/
https://reviews.llvm.org/D143971
Files:
clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/bugprone/string-constructor.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone/string-constructor.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/string-constructor.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/string-constructor.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/string-constructor.cpp
@@ -5,12 +5,13 @@
class allocator {};
template
class char_traits {};
-template , typename A = std::allocator >
+template , typename A = std::allocator>
struct basic_string {
basic_string();
- basic_string(const C*, unsigned int size);
- basic_string(const C *, const A &allocator = A());
- basic_string(unsigned int size, C c);
+ basic_string(const basic_string&, unsigned int, unsigned int, const A & = A());
+ basic_string(const C*, unsigned int);
+ basic_string(const C*, const A& = A());
+ basic_string(unsigned int, C);
};
typedef basic_string string;
typedef basic_string wstring;
@@ -18,7 +19,7 @@
template >
struct basic_string_view {
basic_string_view();
- basic_string_view(const C *, unsigned int size);
+ basic_string_view(const C *, unsigned int);
basic_string_view(const C *);
};
typedef basic_string_view string_view;
@@ -28,20 +29,74 @@
const char* kText = "";
const char kText2[] = "";
extern const char kText3[];
+char getChar();
+const char* getCharPtr();
+
+struct CharLikeObj {
+ operator char() const;
+};
void Test() {
- std::string str('x', 4);
- // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor parameters are probably swapped; expecting string(count, character) [bugprone-string-constructor]
- // CHECK-FIXES: std::string str(4, 'x');
- std::wstring wstr(L'x', 4);
- // CHECK-MESSAGES: [[@LINE-1]]:16: warning: string constructor parameters are probably swapped
- // CHECK-FIXES: std::wstring wstr(4, L'x');
+ short sh;
+ int i;
+ int& ref_i = i;
+ char ch;
+ CharLikeObj char_like_obj;
+
+ std::string swapped('x', 4);
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped(4, 'x');
+ std::wstring wswapped(L'x', 4);
+ // CHECK-MESSAGES: [[@LINE-1]]:16: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::wstring wswapped(4, L'x');
+ std::string swapped2('x', i);
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped2(i, 'x');
+ std::string swapped3(ch, 4);
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped3(4, ch);
+ std::string swapped4(ch, i);
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped4(i, ch);
+ std::string swapped5('x', (int)'x');
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped5((int)'x', 'x');
+ std::string swapped6(getChar(), 10);
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped6(10, getChar());
+ std::string swapped7((('x')), ( i ));
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped7(( i ), (('x')));
+ std::string swapped8((ch), (i));
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped8((i), (ch));
+ std::string swapped9((getChar()), (i));
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped9((i), (getChar()));
std::string s0(0, 'x');
// CHECK-MESSAGES: [[@LINE-1]]:15: warning: constructor creating an empty st
[PATCH] D143971: [clang-tidy] Flag more buggy string constructor cases
ccotter updated this revision to Diff 509541.
ccotter added a comment.
rebase
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143971/new/
https://reviews.llvm.org/D143971
Files:
clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.cpp
clang-tools-extra/clang-tidy/modernize/AvoidCArraysCheck.h
clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.h
clang-tools-extra/clang-tidy/utils/CMakeLists.txt
clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
clang-tools-extra/clang-tidy/utils/TypeUtils.cpp
clang-tools-extra/clang-tidy/utils/TypeUtils.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/modernize/avoid-c-arrays.rst
clang-tools-extra/test/clang-tidy/checkers/modernize/avoid-c-arrays-cxx17.cpp
clang-tools-extra/test/clang-tidy/checkers/modernize/avoid-c-arrays.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/modernize/avoid-c-arrays.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize/avoid-c-arrays.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize/avoid-c-arrays.cpp
@@ -1,4 +1,6 @@
-// RUN: %check_clang_tidy %s modernize-avoid-c-arrays %t
+// RUN: %check_clang_tidy -std=c++11 %s modernize-avoid-c-arrays %t
+
+//CHECK-FIXES: #include
int a[] = {1, 2};
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: do not declare C-style arrays, use std::array<> instead
@@ -86,3 +88,262 @@
int j[1];
};
}
+
+template struct TStruct {};
+
+void replacements() {
+ int ar[10];
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use std::array<> instead
+ // CHECK-FIXES: std::array ar;
+ TStruct ar2[10];
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use std::array<> instead
+ // CHECK-FIXES: std::array, 10> ar2;
+ TStruct< int > ar3[10];
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use std::array<> instead
+ // CHECK-FIXES: std::array, 10> ar3;
+ int * ar4[10];
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use std::array<> instead
+ // CHECK-FIXES: std::array ar4;
+ int * /*comment*/ar5[10];
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use std::array<> instead
+ // CHECK-FIXES: std::array /*comment*/ar5;
+ volatile const int * ar6[10];
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: do not declare C-style arrays, use std::array<> instead
+ // CHECK-FIXES: std::array ar6;
+ volatile int ar7[10];
+ // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not declare C-style arrays, use std::array<> instead
+ // CHECK-FIXES: volatile std::array ar7;
+ int const * ar8[10];
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use std::array<> instead
+ // CHECK-FIXES: std::array ar8;
+ int ar9[1];
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use std::array<> instead
+ // CHECK-FIXES: std::array ar9;
+ static int volatile constexpr ar10[10] = {};
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: do not declare C-style arrays, use std::array<> instead
+ // CHECK-FIXES: static volatile constexpr std::array ar10 = { {} };
+ thread_local int ar11[10];
+ // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: do not declare C-style arrays, use std::array<> instead
+ // CHECK-FIXES: thread_local std::array ar11;
+ thread_local/*a*/int/*b*/ar12[10];
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: do not declare C-style arrays, use std::array<> instead
+ // CHECK-FIXES: thread_local/*a*/std::array/*b*/ar12;
+ /*a*/ int/*b*/ /*c*/*/*d*/ /*e*/ /*f*/ar13[10];
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: do not declare C-style arrays, use std::array<> instead
+ // CHECK-FIXES: /*a*/ std::array/*d*/ /*e*/ /*f*/ar13;
+ TStruct ar14[10];
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use std::array<> instead
+ // CHECK-FIXES: std::array, 10> ar14;
+ volatile TStruct ar15[10];
+ // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not declare C-style arrays, use std::array<> instead
+ // CHECK-FIXES: volatile std::array, 10> ar15;
+ TStruct ar16[10];
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use std::array<> instead
+ // CHECK-FIXES: std::array, 10> ar16;
+ TStruct ar17[10];
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use std::array<> instead
+ // CHECK-FIXES: std::array, 10> ar17;
+ volatile int static thread_local * ar18[10];
+ // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not declare C-style arrays, use std::array<> instead
+ // CHECK-FIXES: static thread_local std::array ar18;
+
+ // Note, there is a tab '\t' before the semicolon in the declaration below.
+ int ar19[3] ;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare
[PATCH] D143971: [clang-tidy] Flag more buggy string constructor cases
ccotter marked an inline comment as done.
ccotter added inline comments.
Comment at:
clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp:70-74
+ const auto CharExpr = expr(anyOf(
+ ignoringParenImpCasts(characterLiteral()),
+
declRefExpr(hasDeclaration(varDecl(hasType(qualType(isAnyCharacter()),
+ ignoringParens(callExpr(
+ callee(functionDecl(returns(qualType(isAnyCharacter();
PiotrZSL wrote:
> add a testcase with:
> ```
> char& chRef = ch;
> std::string swapped2(chRef, i);
>
> char* chPtr = &ch;
> std::string swapped2(*chPtr, i);
>
> using Character = char;
> Character c;
> std::string swapped2(c, i);
> ```
>
> I fear that it may not match those, because here you are trying to match
> specific use cases, when you should simply match a type of expression. In
> such case this should be just:
> ```
> const auto CharExpr =
> expr(hasType(qualType(hasCanonicalType(anyOf(isAnyCharacter(),
> references(isAnyCharacter()));
> ```
>
> Only issue would be implicit cast, you can try deal with them by using
> ignoringImplicit, or in better way just by using:
>
> ```
> traverse(TK_IgnoreUnlessSpelledInSource, hasArgument(0,
> CharExpr.bind("swapped-parameter"))),
> ```
> TK_IgnoreUnlessSpelledInSource will cut of all implicit operations, so you
> could check if called constructor is actually the wrong one, and check
> argument types. Because now macher in line 125 may not always work if somehow
> we would have for example 2 implicit casts, for example from char reference
> to char, or some other crap.
>
> It's up to you, but consider more generic approach instead of matching
> specific use cases like references to variables, or calls.
>
Thinking about this more, I think we have to keep the "args swapped" case very
narrow to the existing patterns of the first arg being a char literal, and the
second being an integer literal. Consider
```
char buffer[] = "abcdef";
char& chRef = ch;
std::string not_swapped(chRef, 4); // Forgot '&' in front of 'chRef'
std::string also_not_swapped(buffer[2], 2); // Forgot '&' in front of
'buffer[2]'
```
Neither of these are swapped arg bugs, so I don't think we can have the tool
consider this a swapped arg bug and apply fixits. The `also_not_swapped` is the
exact bug I wrote a few weeks ago.
I think there are two types of bugs the tool has to be aware of with respect to
the string fill constructor
1. swapped args
2. "confusing" args - where the arguments are both implicitly cast from int to
char and vice versa.
For swapped args, the existing logic will match when the first arg has
`isInteger()` type, and the second arg is `characterLiteral()`. At most, I
think we can only expand the logic to include constructor exprs when the second
arg is a declRefExpr to a `char`, but not char ref (see the `not_swapped`
example above), or when the second arg is the result of a function call that
returns a char. In these cases, we can be sure that the the author did not mean
to put a `&` in front of the second argument expression.
For "confusing" args, which is not implemented in the existing logic, but I am
proposing in this change, this is the more general case where I think the logic
should match any expression as long as both argument expressions are implicitly
cast. This is "confusing" because int-to-char and char-to-int conversions are
not obvious to the reader, and it should be rare for both conversions to take
place when calling the fill constructor (confirmed anecdotally in the codebases
I've checked). The tool could not offer fixits here since the fix might be to
swap the args, or to add a `&` in front of the first arg (or even another
change). At most, we can warn and alert the user to the problem. If this is
what the author intended, they can use explicit casts to make the intent
obvious to the reader.
Comment at: clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp:95
+ const auto NonCharacterInteger =
+ qualType(isInteger(), unless(isAnyCharacter()));
+ const auto CharToIntCastExpr = implicitCastExpr(
PiotrZSL wrote:
> what about references to integer, or typedefs to integers ?
I'll add a test for reference to integer, but I'm matching on the type of the
source expression that is being cast, which will be of integer type, even if
the expression is `string str(x, y)` and `y` is an `int&` (the source
expression in the cast will be of type `int`).
Comment at: clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp:213
}
+ } else if (const auto *E =
+ Result.Nodes.getNodeAs("implicit-cast-both-args")) {
PiotrZSL wrote:
> i thing that this case should be matched by "swapped-parameter", no need to
> add extra one, just first one should be fixed.
See earlier explanation on why we can't treat these all as swapped parameters
cases.
=
[PATCH] D143971: [clang-tidy] Flag more buggy string constructor cases
ccotter updated this revision to Diff 508345.
ccotter marked 4 inline comments as done.
ccotter added a comment.
- Add more cases to swapped params
- Change message, add more tests
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143971/new/
https://reviews.llvm.org/D143971
Files:
clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/bugprone/string-constructor.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone/string-constructor.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/string-constructor.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/string-constructor.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/string-constructor.cpp
@@ -5,12 +5,13 @@
class allocator {};
template
class char_traits {};
-template , typename A = std::allocator >
+template , typename A = std::allocator>
struct basic_string {
basic_string();
- basic_string(const C*, unsigned int size);
- basic_string(const C *, const A &allocator = A());
- basic_string(unsigned int size, C c);
+ basic_string(const basic_string&, unsigned int, unsigned int, const A & = A());
+ basic_string(const C*, unsigned int);
+ basic_string(const C*, const A& = A());
+ basic_string(unsigned int, C);
};
typedef basic_string string;
typedef basic_string wstring;
@@ -18,7 +19,7 @@
template >
struct basic_string_view {
basic_string_view();
- basic_string_view(const C *, unsigned int size);
+ basic_string_view(const C *, unsigned int);
basic_string_view(const C *);
};
typedef basic_string_view string_view;
@@ -28,20 +29,74 @@
const char* kText = "";
const char kText2[] = "";
extern const char kText3[];
+char getChar();
+const char* getCharPtr();
+
+struct CharLikeObj {
+ operator char() const;
+};
void Test() {
- std::string str('x', 4);
- // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor parameters are probably swapped; expecting string(count, character) [bugprone-string-constructor]
- // CHECK-FIXES: std::string str(4, 'x');
- std::wstring wstr(L'x', 4);
- // CHECK-MESSAGES: [[@LINE-1]]:16: warning: string constructor parameters are probably swapped
- // CHECK-FIXES: std::wstring wstr(4, L'x');
+ short sh;
+ int i;
+ int& ref_i = i;
+ char ch;
+ CharLikeObj char_like_obj;
+
+ std::string swapped('x', 4);
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped(4, 'x');
+ std::wstring wswapped(L'x', 4);
+ // CHECK-MESSAGES: [[@LINE-1]]:16: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::wstring wswapped(4, L'x');
+ std::string swapped2('x', i);
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped2(i, 'x');
+ std::string swapped3(ch, 4);
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped3(4, ch);
+ std::string swapped4(ch, i);
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped4(i, ch);
+ std::string swapped5('x', (int)'x');
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped5((int)'x', 'x');
+ std::string swapped6(getChar(), 10);
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped6(10, getChar());
+ std::string swapped7((('x')), ( i ));
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped7(( i ), (('x')));
+ std::string swapped8((ch), (i));
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped8((i), (ch));
+ std::string swapped9((getChar()), (i));
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped9((i), (getChar()));
[PATCH] D143971: [clang-tidy] Flag more buggy string constructor cases
carlosgalvezp added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone/string-constructor.rst:25 + std::string str(buf[1], 5); // First arg should be '&buf[1]'? + std::string str2((int)buf[1], 5); // Ok - explicitly cast to express intent Should we use C++ casts? Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone/string-constructor.rst:25 + std::string str(buf[1], 5); // First arg should be '&buf[1]'? + std::string str2((int)buf[1], 5); // Ok - explicitly cast to express intent carlosgalvezp wrote: > Should we use C++ casts? Should this be a char? Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone/string-constructor.cpp:141 + std::string s4((int)kText[1], i); + std::string s5(kText[1], (char)i); Shouldn't this fail, since the constructor `std::string(char, char)` does not exist? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143971/new/ https://reviews.llvm.org/D143971 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D143971: [clang-tidy] Flag more buggy string constructor cases
PiotrZSL added inline comments.
Comment at: clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp:97
+ const auto CharToIntCastExpr = implicitCastExpr(
+ hasSourceExpression(expr(hasType(qualType(isAnyCharacter(),
+ hasImplicitDestinationType(NonCharacterInteger));
PiotrZSL wrote:
> reuse here CharExpr
try with test like this:
```
struct Class
{
operator char() const;
};
Class c;
std::string value(c, 5);
```
I fear that hasSourceExpression could match Class type, and therefore this case
wouldn't be detected.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143971/new/
https://reviews.llvm.org/D143971
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D143971: [clang-tidy] Flag more buggy string constructor cases
PiotrZSL added inline comments.
Comment at:
clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp:70-74
+ const auto CharExpr = expr(anyOf(
+ ignoringParenImpCasts(characterLiteral()),
+
declRefExpr(hasDeclaration(varDecl(hasType(qualType(isAnyCharacter()),
+ ignoringParens(callExpr(
+ callee(functionDecl(returns(qualType(isAnyCharacter();
add a testcase with:
```
char& chRef = ch;
std::string swapped2(chRef, i);
char* chPtr = &ch;
std::string swapped2(*chPtr, i);
using Character = char;
Character c;
std::string swapped2(c, i);
```
I fear that it may not match those, because here you are trying to match
specific use cases, when you should simply match a type of expression. In such
case this should be just:
```
const auto CharExpr =
expr(hasType(qualType(hasCanonicalType(anyOf(isAnyCharacter(),
references(isAnyCharacter()));
```
Only issue would be implicit cast, you can try deal with them by using
ignoringImplicit, or in better way just by using:
```
traverse(TK_IgnoreUnlessSpelledInSource, hasArgument(0,
CharExpr.bind("swapped-parameter"))),
```
TK_IgnoreUnlessSpelledInSource will cut of all implicit operations, so you
could check if called constructor is actually the wrong one, and check argument
types. Because now macher in line 125 may not always work if somehow we would
have for example 2 implicit casts, for example from char reference to char, or
some other crap.
It's up to you, but consider more generic approach instead of matching specific
use cases like references to variables, or calls.
Comment at: clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp:95
+ const auto NonCharacterInteger =
+ qualType(isInteger(), unless(isAnyCharacter()));
+ const auto CharToIntCastExpr = implicitCastExpr(
what about references to integer, or typedefs to integers ?
Comment at: clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp:97
+ const auto CharToIntCastExpr = implicitCastExpr(
+ hasSourceExpression(expr(hasType(qualType(isAnyCharacter(),
+ hasImplicitDestinationType(NonCharacterInteger));
reuse here CharExpr
Comment at: clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp:213
}
+ } else if (const auto *E =
+ Result.Nodes.getNodeAs("implicit-cast-both-args")) {
i thing that this case should be matched by "swapped-parameter", no need to add
extra one, just first one should be fixed.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143971/new/
https://reviews.llvm.org/D143971
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D143971: [clang-tidy] Flag more buggy string constructor cases
ccotter updated this revision to Diff 500645.
ccotter added a comment.
- Add more cases to swapped params
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143971/new/
https://reviews.llvm.org/D143971
Files:
clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/bugprone/string-constructor.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone/string-constructor.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/string-constructor.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/string-constructor.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/string-constructor.cpp
@@ -5,12 +5,13 @@
class allocator {};
template
class char_traits {};
-template , typename A = std::allocator >
+template , typename A = std::allocator>
struct basic_string {
basic_string();
- basic_string(const C*, unsigned int size);
- basic_string(const C *, const A &allocator = A());
- basic_string(unsigned int size, C c);
+ basic_string(const basic_string&, unsigned int, unsigned int, const A & = A());
+ basic_string(const C*, unsigned int);
+ basic_string(const C*, const A& = A());
+ basic_string(unsigned int, C);
};
typedef basic_string string;
typedef basic_string wstring;
@@ -18,7 +19,7 @@
template >
struct basic_string_view {
basic_string_view();
- basic_string_view(const C *, unsigned int size);
+ basic_string_view(const C *, unsigned int);
basic_string_view(const C *);
};
typedef basic_string_view string_view;
@@ -28,20 +29,58 @@
const char* kText = "";
const char kText2[] = "";
extern const char kText3[];
+char getChar();
+const char* getCharPtr();
void Test() {
- std::string str('x', 4);
- // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor parameters are probably swapped; expecting string(count, character) [bugprone-string-constructor]
- // CHECK-FIXES: std::string str(4, 'x');
- std::wstring wstr(L'x', 4);
- // CHECK-MESSAGES: [[@LINE-1]]:16: warning: string constructor parameters are probably swapped
- // CHECK-FIXES: std::wstring wstr(4, L'x');
+ short sh;
+ int i;
+ char ch;
+
+ std::string swapped('x', 4);
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped(4, 'x');
+ std::wstring wswapped(L'x', 4);
+ // CHECK-MESSAGES: [[@LINE-1]]:16: warning: string constructor arguments are probably swapped
+ // CHECK-FIXES: std::wstring wswapped(4, L'x');
+ std::string swapped2('x', i);
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped2(i, 'x');
+ std::string swapped3(ch, 4);
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped3(4, ch);
+ std::string swapped4(ch, i);
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped4(i, ch);
+ std::string swapped5('x', (int)'x');
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped5((int)'x', 'x');
+ std::string swapped6(getChar(), 10);
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped6(10, getChar());
+ std::string swapped7((('x')), ( i ));
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped7(( i ), (('x')));
+ std::string swapped8((ch), (i));
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped8((i), (ch));
+ std::string swapped9((getChar()), (i));
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: string constructor arguments are probably swapped; expecting string(count, character) [bugprone-string-constructor]
+ // CHECK-FIXES: std::string swapped9((i), (getChar()));
std::string s0(0, 'x');
// CHECK-MESSAGES: [[@LINE-1]]:15: warning: constructor creating an empty string
std::string s1(-4, 'x');
// CHECK-MESSAGES: [[@LINE-1]]:15: warning: negative value used as length parameter
std::string s2(0x1
