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