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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
