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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth marked an inline comment as done.
JonasToth added inline comments.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp:608
+}
+
+template <typename L, typename R>
----------------
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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943