Compile warnings on Windows with MinGW

[pali]

https://travis-ci.org/Dual-Life/Devel-PPPort/jobs/546411085

In file included from RealPPPort.xs:31:
RealPPPort.xs: In function 'XS_Devel__PPPort_ptrtests':
C:\STRAWB~1\perl\lib\CORE/perl.h:1780:26: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
 #  define INT2PTR(any,d) (any)(d)
                          ^
C:\STRAWB~1\perl\lib\CORE/perl.h:1793:21: note: in expansion of macro 'INT2PTR'
 #  define PTR2ul(p) INT2PTR(unsigned long,p)
                     ^~~~~~~
RealPPPort.xs:1686:27: note: in expansion of macro 'PTR2ul'
                 RETVAL += PTR2ul(p) != 0UL      ?  2 : 0;
                           ^~~~~~

[khwilliamson]

@pali suggestions for fixing. I would think this is a problem in blead as well, not just for this module.

[sisyphus]

I would think this is a problem in blead as well, not just for this module.

Indeed - the same issue has resurfaced at Perl/perl5#20080 .

This patch to parts/inc/misc avoids the warnings and appears to be functionally correct to me. (The Devel-PPPort test suite still passes all tests for me on 64-bit windows, 32-bit Windows and 64-bit Ubuntu.)

--- misc_orig   2022-08-16 15:14:57.318512000 +1000
+++ misc_mod    2022-08-16 14:45:11.000000000 +1000
@@ -1129,6 +1129,13 @@

 =xsmisc

+/* Avoid PTR2ul calls in the code if PTRSIZE > LONGSIZE */
+#if PTRSIZE > LONGSIZE
+#  define PTR2U(p) PTR2UV(p)
+#else
+#  define PTR2U(p) PTR2ul(p)
+#endif
+
 typedef XSPROTO(XSPROTO_test_t);
 typedef XSPROTO_test_t *XSPROTO_test_t_ptr;

@@ -1267,7 +1274,12 @@
         CODE:
                 RETVAL = 0;
                 RETVAL += PTR2nat(p) != 0       ?  1 : 0;
-                RETVAL += PTR2ul(p) != 0UL      ?  2 : 0;
+
+                /* If PTRSIZE > LONGSIZE PTR2U(p) is defined
+                 * as PTR2UV(p) as PTR2ul(p) is inappropriate.
+                 * Else PTR2U(p) is defined as PTR2ul(p) */
+                RETVAL += PTR2U(p)  != 0UL      ?  2 : 0;
+
                 RETVAL += PTR2UV(p) != (UV) 0   ?  4 : 0;
                 RETVAL += PTR2IV(p) != (IV) 0   ?  8 : 0;
                 RETVAL += PTR2NV(p) != (NV) 0   ? 16 : 0;

If there are no requests for alterations, I'll submit it as a PR.

It's a little bit inefficient in that, on a system where PTRSIZE > LONGSIZE, PTR2UV{p) is evaluated twice.
In the first instance it is compared with 0UL - and RETVAL is incremented by 2 if that comparison returns TRUE.
In the second instance it is compared with (UV)0 and RETVAL is incremented by 4 if that comparison returns TRUE.
But we're only running tests, so hopefully this duplication can be overlooked.

Cheers,
Rob

[sisyphus]

I think that, in the absence of any corrections, the next thing to do is to create a new pull request for this at https://github.com/Perl/perl5/pulls.

If no-one else wants to create that PR, I'm happy to do it.
I think the comments in the patch could be rewritten to add a little clarity:

/* If PTRSIZE > LONGSIZE, PTR2U(p) is defined
  * as PTR2UV(p) because PTR2ul(p) is strictly
  * inappropriate and emits compilation warnings.
  * Else PTR2U(p) is defined as PTR2ul(p) */

[khwilliamson]

It's easier to comment on an actual PR, but here are some. Maybe a real PR would take away the ambiguity

I don't understand this, actually. I do believe that a UV is guaranteed to be large enough to hold any pointer on the platform as compiled. So if, an unsigned long isn't big enough, what is? I think only a UV (if we had 128bit words, a quad could be an intermediate step).

Are IVSIZE and UVSIZE ever different?

And, unless I'm being fooled, the CODE: is merely for a test function; efficiency doesn't matte, and I don't see how it would help a general user. PTRU does not look to me to be an API element, nor should it. Thus it needs to follow the naming conventions detailed in HACKERS

[sisyphus]

I do believe that a UV is guaranteed to be large enough to hold any pointer on the platform as compiled

Yes - but with 64-bit builds on MS Windows, although ptrsize and uvsize are 8, longsize is still only 4.
The UV is actually the 8-byte unsigned long long, not the 4-byte unsigned long.
I don't know if there are other systems where a similar thing happens. I just wanted to make the patch all-encompassing, in case there are.

PTRU does not look to me to be an API element, nor should it. Thus it needs to follow the naming conventions detailed in HACKERS.

Ah - yes I overlooked that there might be "naming conventions" to consider here. But I've now looked in perlhack couldn't spot any info about it.
Do you have more specific information on where it could be found ?
Or even just a suggestion on what my "PTR2U" should be changed to would probably suffice for now.

CODE: is merely for a test function; efficiency doesn't matter

Agreed - and I remarked as such.
I just wanted to point it out in case it might be a point of contention to someone.

Cheers,
Rob