do_print: Use new utf8_to_bytes_temp_pv() #22812
Conversation
khwilliamson
commented
Dec 3, 2024
khwilliamson
commented
- This set of changes does not require a perldelta entry.
khwilliamson
changed the title
doio.c
Outdated
| STRLEN tmplen = len; | ||
| bool utf8 = TRUE; | ||
| U8 * const result = bytes_from_utf8((const U8*) tmps, &tmplen, &utf8); | ||
| if (!utf8) { | ||
|
|
||
| /* Here, succeeded in downgrading from utf8. Set up to below | ||
| * output the converted value */ | ||
| tmpbuf = result; | ||
| tmps = (char *) tmpbuf; | ||
| len = tmplen; | ||
| } | ||
| else { /* Non-utf8 output stream, but string only representable in | ||
| utf8 */ | ||
| assert((char *)result == tmps); | ||
| if (! utf8_to_bytes_temp_pv((const U8**) &tmps, &len)) { | ||
| /* Non-utf8 output stream, but string only representable in | ||
| utf8 */ |
There was a problem hiding this comment.
This has a similar problem to do_chomp() - print may be called with a large list, which could leave a large number of temporary allocations active beyond their use until the next LEAVE, and significantly expand the save stack.
The function doesn't violate const, and this means one doesn't have to cast away const unnecessarily.
This removes 6 casts and adds 2, resulting in cleaner looking code.
These new functions do the work this used to duplicate, except they allocate memory only if necessary, instead of always, as previously
| @@ -3716,7 +3716,7 @@ Cp |bool |utf8_to_bytes_ |NN U8 **s_ptr \ | |||
| Admp |bool |utf8_to_bytes_new_pv \ | |||
| |NN U8 const **s_ptr \ | |||
| |NN STRLEN *lenp \ | |||
| |NN U8 *free_me | |||
| |NN const U8 *free_me | |||
There was a problem hiding this comment.
I may have missed this, since it's purely a documentation issue, but free_me isn't a pointer to U8, it certainly isn't a pointer to const U8.
free_me would ideally be declared as void ** but the existing users are passing in a U8 ** which makes that more reasonable here.
And it shouldn't be const in any way, free() requires a non-const pointer, it's only a bug that Safefree() casts const away. Your modification to the macro below also casts const away.
Using inline functions instead of macros here would have made the type mismatches a lot more obvious.
| @@ -2214,36 +2214,37 @@ Perl_do_print(pTHX_ SV *sv, PerlIO *fp) | |||
| else { | |||
| STRLEN len; | |||
| /* Do this first to trigger any overloading. */ | |||
| const char *tmps = SvPV_const(sv, len); | |||
| const U8 *tmps = (U8 *) SvPV_const(sv, len); | |||
There was a problem hiding this comment.
For consistency the cast should be to (const U8 *), since the rest of SvPV_const() is a const pointer, you're casting const away just to add it back in. (makes me wish for C++ const_cast, static_cast etc)
| @@ -2259,7 +2260,7 @@ Perl_do_print(pTHX_ SV *sv, PerlIO *fp) | |||
| * but only until the system hard limit/the filesystem limit, | |||
| * at which we would get EPERM. Note that when using buffered | |||
| * io the write failure can be delayed until the flush/close. --jhi */ | |||
| if (len && (PerlIO_write(fp,tmps,len) == 0)) | |||
| if (len && (PerlIO_write(fp,(char *) tmps,len) == 0)) | |||
There was a problem hiding this comment.
PerlIO_write() takes a const void * for the buffer, you don't need the cast.
| * allocated memory that tmps will also be changed to point to, so | ||
| * Safefree(free_me) will free it. This saves having to have extra | ||
| * logic. */ | ||
| const U8 *free_me = NULL; |
There was a problem hiding this comment.
Per my comments on the other commits, this should not be const.
