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

Bhuminjay Soni via cfe-commits cfe-commits at lists.llvm.org
Tue Jan 30 01:21:14 PST 2024


11happy wrote:

> 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.

@PiotrZSL  I have attempted to implement this suggestion, but I am encountering difficulties and confusion due to the various types involved. Could you kindly provide some guidance or assistance on how to effectively simplify the type hierarchy in this context? I have explored methods like getDesugaredType() but would appreciate your guidance specifically 

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


More information about the cfe-commits mailing list