[clang-tools-extra] [llvm] [clang] Add clang-tidy check to suggest replacement of conditional statement with std::min/std::max (PR #77816)

Piotr Zegar via cfe-commits cfe-commits at lists.llvm.org
Sun Jan 28 22:34:22 PST 2024


https://github.com/PiotrZSL commented:

Other thing is that reviewer should mark issues as resolved after checking with code, not author. Otherwise is hard to figure out what's done and what's not. Adding some comment like "Done" is sufficient usually unless issues are simple.

Next thing regarding your 2 comments that somehow cannot find in GitHub and exist only in my mail box:

1. vector size
```
  std::vector<std::vector<Foo>> a;
    unsigned int b = a[0].size();
    if(a[0].size() > b)
        b = a[0].size();
```
first, you do not have a test for that, like:
```
using my_size = unsigned long long;

template<typename T>
struct MyVector
{
    using size_type = my_size;
    size_type size() const;
};

void testVectorSizeType() {
  MyVector<int> v;

  unsigned int value;
  if (v.size() > value)
    value = v.size();

  if (value < v.size())
    value = v.size();
}
```
Second, in this case check in current state work fine and will put unsigned long long as a type, the only problem is that unsigned long long is ugly.
```
ElaboratedType 0x55cfafb2f390 'size_type' sugar
`-TypedefType 0x55cfafb2f360 'MyVector::size_type' sugar
  |-TypeAlias 0x55cfafb2f300 'size_type'
  `-ElaboratedType 0x55cfafb2f2c0 'my_size' sugar
    `-TypedefType 0x55cfafb2f290 'my_size' sugar
      |-TypeAlias 0x55cfafb2d900 'my_size'
      `-BuiltinType 0x55cfafabb710 'unsigned long long'
```
This is multi level typedef, proper solution would be to drop elaborated type until we get into alias that isn't template instance and isn't in template instance and isn't in template.. 
You can look into readability-redundant-casting how to drop some levels of elaborated types.  And use that instead of getCanonicalType.

2. `value1 + value2 < valuex`
You should check only that binary operator that check trigger:
```
IfStmt 0x56422decb568 <line:256:3, line:257:23>
      |-BinaryOperator 0x56422decb430 <line:256:6, col:24> 'bool' '<'
      | |-ImplicitCastExpr 0x56422decb418 <col:6, col:15> 'unsigned int' <IntegralCast>
      | | `-BinaryOperator 0x56422decb3c0 <col:6, col:15> 'int' '+'
      | |   |-ImplicitCastExpr 0x56422decb378 <col:6> 'int' <IntegralCast>
      | |   | `-ImplicitCastExpr 0x56422decb360 <col:6> 'unsigned char' <LValueToRValue>
      | |   |   `-DeclRefExpr 0x56422decb320 <col:6> 'unsigned char' lvalue Var 0x56422decb0c8 'value1' 'unsigned char'
      | |   `-ImplicitCastExpr 0x56422decb3a8 <col:15> 'int' <IntegralCast>
      | |     `-ImplicitCastExpr 0x56422decb390 <col:15> 'short' <LValueToRValue>
      | |       `-DeclRefExpr 0x56422decb340 <col:15> 'short' lvalue Var 0x56422decb198 'value2' 'short'
      | `-ImplicitCastExpr 0x56422decb400 <col:24> 'unsigned int' <LValueToRValue>
      |   `-DeclRefExpr 0x56422decb3e0 <col:24> 'unsigned int' lvalue Var 0x56422decb268 'valuex' 'unsigned int'
      `-BinaryOperator 0x56422decb548 <line:257:5, col:23> 'unsigned int' lvalue '='
        |-DeclRefExpr 0x56422decb450 <col:5> 'unsigned int' lvalue Var 0x56422decb268 'valuex' 'unsigned int'
        `-ImplicitCastExpr 0x56422decb530 <col:14, col:23> 'unsigned int' <IntegralCast>
          `-BinaryOperator 0x56422decb510 <col:14, col:23> 'int' '+'
            |-ImplicitCastExpr 0x56422decb4c8 <col:14> 'int' <IntegralCast>
            | `-ImplicitCastExpr 0x56422decb4b0 <col:14> 'unsigned char' <LValueToRValue>
            |   `-DeclRefExpr 0x56422decb470 <col:14> 'unsigned char' lvalue Var 0x56422decb0c8 'value1' 'unsigned char'
            `-ImplicitCastExpr 0x56422decb4f8 <col:23> 'int' <IntegralCast>
              `-ImplicitCastExpr 0x56422decb4e0 <col:23> 'short' <LValueToRValue>
                `-DeclRefExpr 0x56422decb490 <col:23> 'short' lvalue Var 0x56422decb198 'value2' 'short'
```
in this case we got: `unsigned char + short < unsigned int`, first part will be cased to `unsigned int` even if ``unsigned char + short` gives int, and that's fine. Current version of check handles that well, `GlobalImplicitCastType = BO->getLHS()->getType();` does a trick.

```

https://github.com/llvm/llvm-project/pull/77816


More information about the cfe-commits mailing list