[PATCH] D143971: [clang-tidy] Flag more buggy string constructor cases

[Piotr Zegar via Phabricator via cfe-commits]

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:3, col:27> col:15 str2 'std::string':'std::basic_string<char>' callinit
  |       `-CXXConstructExpr 0x56436ceb0650 <col:15, col:27> 'std::string':'std::basic_string<char>' 'void (unsigned int, char)'
  |         |-ImplicitCastExpr 0x56436ceb0608 <col:20> 'unsigned int' <IntegralCast>
  |         | `-ImplicitCastExpr 0x56436ceb05f0 <col:20> 'char' <LValueToRValue>
  |         |   `-DeclRefExpr 0x56436ceb0588 <col:20> 'char' lvalue Var 0x56436ceb0280 'c' 'char'
  |         `-ImplicitCastExpr 0x56436ceb0638 <col:23> 'char':'char' <IntegralCast>
  |           `-ImplicitCastExpr 0x56436ceb0620 <col:23> 'Size':'int' <LValueToRValue>
  |             `-DeclRefExpr 0x56436ceb05a8 <col:23> '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