Thread: [HACKERS] compiler warning with VS 2017
I am getting a compiler warning when I build the latest HEAD PostgreSQL with
visual studio 2017.
src/backend/replication/logical/proto.c(482): warning C4312: 'type cast': conversion from 'unsigned int' to 'char *' of greater size
Details of the warning is available in the link [1].
The code at the line is,
tuple->values[i] = (char *) (Size)0xdeadbeef; /* make bad usage more obvious */
If I change the code as (char *) (Size)0xdeadbeef;
or (char *) (INT_PTR)0xdeadbeef; the warning disappears.
How about fixing it as below and add the typecast or disable
this warning?
/*
* PTR_CAST
* Used when converting integer addresses to a pointer.
* The casting is used to avoid generating warning in
* windows
*/
#ifdef WIN32
#define PTR_CAST INT_PTR
#else
#define PTR_CAST
#endif
Regards,
Hari Babu
Fujitsu Australia
Haribabu Kommi <kommi.haribabu@gmail.com> writes: > I am getting a compiler warning when I build the latest HEAD PostgreSQL with > visual studio 2017. > The code at the line is, > tuple->values[i] = (char *) (Size)0xdeadbeef; /* make bad usage more obvious */ Yeah, you're not the first to complain about this. To my mind that coding is not pretty, not cute, and not portable: there's not even a good reason to believe that dereferencing the pointer would result in a crash. Perhaps the author can explain to us why this is better than just assigning NULL. Actually, looking around a bit there, it's not even clear why we should be booby-trapping the value of an unchanged column in the first place. So I'd say that not only is the code dubious but the comment is inadequate too. regards, tom lane
On 05/05/17 06:50, Tom Lane wrote: > Haribabu Kommi <kommi.haribabu@gmail.com> writes: >> I am getting a compiler warning when I build the latest HEAD PostgreSQL with >> visual studio 2017. >> The code at the line is, >> tuple->values[i] = (char *) (Size)0xdeadbeef; /* make bad usage more obvious */ > > Yeah, you're not the first to complain about this. To my mind that > coding is not pretty, not cute, and not portable: there's not even > a good reason to believe that dereferencing the pointer would result > in a crash. Perhaps the author can explain to us why this is better > than just assigning NULL. > > Actually, looking around a bit there, it's not even clear why > we should be booby-trapping the value of an unchanged column in > the first place. So I'd say that not only is the code dubious > but the comment is inadequate too. Hmm, as far as I can recollect this is just leftover debugging code that was intended to help ensure that we are checking the "changed" everywhere we are supposed to (since I changed handling of these structured quite a bit during development). Should be changed to NULL, that's what we usually do in this type of situation. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Petr Jelinek <petr.jelinek@2ndquadrant.com> writes: > On 05/05/17 06:50, Tom Lane wrote: >> Actually, looking around a bit there, it's not even clear why >> we should be booby-trapping the value of an unchanged column in >> the first place. So I'd say that not only is the code dubious >> but the comment is inadequate too. > Hmm, as far as I can recollect this is just leftover debugging code that > was intended to help ensure that we are checking the "changed" > everywhere we are supposed to (since I changed handling of these > structured quite a bit during development). Should be changed to NULL, > that's what we usually do in this type of situation. So the comment should be something like "if the column is unchanged, we should not attempt to access its value beyond this point. To help catch any such attempts, set the string to NULL" ? regards, tom lane
On 05/05/17 16:56, Tom Lane wrote: > Petr Jelinek <petr.jelinek@2ndquadrant.com> writes: >> On 05/05/17 06:50, Tom Lane wrote: >>> Actually, looking around a bit there, it's not even clear why >>> we should be booby-trapping the value of an unchanged column in >>> the first place. So I'd say that not only is the code dubious >>> but the comment is inadequate too. > >> Hmm, as far as I can recollect this is just leftover debugging code that >> was intended to help ensure that we are checking the "changed" >> everywhere we are supposed to (since I changed handling of these >> structured quite a bit during development). Should be changed to NULL, >> that's what we usually do in this type of situation. > > So the comment should be something like "if the column is unchanged, > we should not attempt to access its value beyond this point. To > help catch any such attempts, set the string to NULL" ? > Yes that sounds about right. We don't get any data for unchanged TOAST columns (that's limitation of logical decoding) so we better not touch them. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Petr Jelinek <petr.jelinek@2ndquadrant.com> writes: > On 05/05/17 16:56, Tom Lane wrote: >> So the comment should be something like "if the column is unchanged, >> we should not attempt to access its value beyond this point. To >> help catch any such attempts, set the string to NULL" ? > Yes that sounds about right. We don't get any data for unchanged TOAST > columns (that's limitation of logical decoding) so we better not touch them. OK. I just made it say "we don't get the value of unchanged columns". regards, tom lane