JonasToth marked 4 inline comments 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>
----------------
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.
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
