[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

Sun Jan 5 08:03:25 PST 2020

0x8000-0000 marked an inline comment as done.
0x8000-0000 added inline comments.

================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp:608
+}
+
+template <typename L, typename R>
----------------
JonasToth wrote:
> JonasToth wrote:
> > 0x8000-0000 wrote:
> > > JonasToth wrote:
> > > > 0x8000-0000 wrote:
> > > > > 0x8000-0000 wrote:
> > > > > > JonasToth wrote:
> > > > > > > JonasToth wrote:
> > > > > > > > 0x8000-0000 wrote:
> > > > > > > > > Please insert the this test code here:
> > > > > > > > > 
> > > > > > > > > ```
> > > > > > > > > struct IntWrapper {
> > > > > > > > > 
> > > > > > > > >    unsigned low;
> > > > > > > > >    unsigned high;
> > > > > > > > > 
> > > > > > > > >    IntWrapper& operator=(unsigned value) {
> > > > > > > > >       low = value & 0xffff;
> > > > > > > > >       high = (value >> 16) & 0xffff;
> > > > > > > > >    }
> > > > > > > > > 
> > > > > > > > >    template<typename Istream>
> > > > > > > > >    friend Istream& operator>>(Istream& is, IntWrapper& rhs) {
> > > > > > > > >       unsigned someValue = 0;       // false positive now
> > > > > > > > >       if (is >> someValue) {
> > > > > > > > >          rhs = someValue;
> > > > > > > > >       }
> > > > > > > > >       return is;
> > > > > > > > >    }
> > > > > > > > > };
> > > > > > > > > 
> > > > > > > > > unsigned TestHiddenFriend(IntMaker& im) {
> > > > > > > > >    IntWrapper iw;
> > > > > > > > > 
> > > > > > > > >    im >> iw;
> > > > > > > > > 
> > > > > > > > >    return iw.low;
> > > > > > > > > }
> > > > > > > > > ```
> > > > > > > > clang gives me no error when I add the const there. sure it does reproduce properly?
> > > > > > > Here for reference: https://godbolt.org/z/DXKMYh
> > > > > > Probably I wasn't clear - I suggested adding my test code at line 608, because it needs the "IntMaker" definition at line 594.
> > > > > > 
> > > > > > The false positive from this const check is reported on the "unsigned someValue = 0;" line inside the template extraction operator.
> > > > > Oh, I got it - don't think "shift" think "overloaded extraction operator".
> > > > > 
> > > > > In my code above, you don't know what ">>" does, and it clearly takes a mutable reference as the right side.
> > > > > 
> > > > > ```
> > > > > const int foo;
> > > > > std::cin >> foo;
> > > > > ```
> > > > > 
> > > > > should not compile, either :)
> > > > no. something else is going on.
> > > > https://godbolt.org/z/xm8eVC
> > > > ```
> > > > | |   |-CXXOperatorCallExpr <line:21:5, col:11> '<dependent type>'
> > > > | |   | |-UnresolvedLookupExpr <col:8> '<overloaded function type>' lvalue (ADL) = 'operator>>' 0x55a26b885938 0x55a26b857238
> > > > | |   | |-DeclRefExpr <col:5> 'Istream' lvalue ParmVar 0x55a26b885748 'is' 'Istream &'
> > > > | |   | `-DeclRefExpr <col:11> 'const unsigned int' lvalue Var 0x55a26b885a38 'someValue' 'const unsigned int'
> > > > ```
> > > > This code is only a false positive in the sense, that the "we can not know if something bad happens" is not detected. The operator>> is not resolved. That is because the template is not instantiated and the expressions can therefore not be resolved yet.
> > > > There seems to be no instantiation of this template function.
> > > > 
> > > > Somehow the `im >> iw;` does not instantiate the `friend operator<<`. I reduced it to something i think is minimal and that shows the same behaviour. (https://godbolt.org/z/MMG_4q)
> > > https://godbolt.org/z/7QEdHJ
> > > 
> > > ```
> > > struct IntMaker {
> > >   operator bool() const;
> > > 
> > >   friend IntMaker &operator>>(IntMaker &, unsigned &);
> > >   //friend IntMaker &operator>>(IntMaker &, const unsigned &) = delete;
> > > };
> > > ```
> > > 
> > > If you uncomment the deleted overload then
> > > 
> > > ```
> > > template <typename Istream>
> > > Istream& operator>>(Istream& is, IntWrapper& rhs)  {
> > >     unsigned const someValue = 0;
> > >     if (is >> someValue) {
> > >         rhs = someValue;
> > >     }
> > >     return is;
> > > }
> > > ```
> > > 
> > > Fails to compile.
> > > 
> > > Depending on what else is around, it seems that somehow the compiler would try to use the (bool) conversion to obtain an integral then use it as an argument to the built-in integral left shift.
> > > 
> > > https://godbolt.org/z/-JFL5o - this does not compile, as expected:
> > > 
> > > ```
> > > #include <iostream>
> > > 
> > > int readInt() {
> > >     const int foo = 0;
> > >     std::cin >> foo;
> > >     return foo;
> > > }
> > > ```
> > > 
> > > so this check should not recommend making foo constant.
> > I see. Implicit conversions are great... :)
> > 
> > I will recheck that. And the `std::cin` example is analyzed correctly. I added a test for that, too.
> > Thank you for researching the issue!
> https://godbolt.org/z/VerWce
> Minimal example that shows the issue.
As much as I would have liked to contribute a good test to your new checker, I'm happier that I have found something strange with my original code from which I was trying to distill the minimal case.

For what it's worth, dumping the AST on my original code, shows this
```
|     | |       |-IfStmt 0x7fdafcfb3a60 <line:60:9, line:63:9>
|     | |       | |-BinaryOperator 0x7fdafcfb32b0 <line:60:13, col:19> '<dependent type>' '>>'
|     | |       | | |-DeclRefExpr 0x7fdafcfb3270 <col:13> 'Istream' lvalue ParmVar 0x7fdafcfb1448 'is' 'Istream &'
|     | |       | | `-DeclRefExpr 0x7fdafcfb3290 <col:19> 'uint32_t':'unsigned int' lvalue Var 0x7fdafcfb31b8 'someValue' 'uint32_t':'unsigned int'
```

Instead of the expected:
```
2220 | |   |   |-IfStmt 0x5971b00 <line:624:7, line:626:7>                                                                                                   
2221 | |   |   | |-CXXOperatorCallExpr 0x596f040 <line:624:11, col:17> '<dependent type>'
2222 | |   |   | | |-UnresolvedLookupExpr 0x596efe8 <col:14> '<overloaded function type>' lvalue (ADL) = 'operator>>' 0x596b8a8 0x596b668 0x596ab38          
2223 | |   |   | | |-DeclRefExpr 0x596efa8 <col:11> 'Istream' lvalue ParmVar 0x596e968 'is' 'Istream &'                                                      
2224 | |   |   | | `-DeclRefExpr 0x596efc8 <col:17> 'unsigned int' lvalue Var 0x596eef0 'someValue' 'unsigned int'
```

I need to investigate if I need to shuffle some includes around to get clang to treat it as an overloaded method instead of shift. At any rate, the new const-checker has proved its worth by pointing me to a bug!

Thank you, Jonas!

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54943/new/

https://reviews.llvm.org/D54943