Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

do_print: Use new utf8_to_bytes_temp_pv() #22812

Open
wants to merge 3 commits into
base: blead
Choose a base branch
from
Open

do_print: Use new utf8_to_bytes_temp_pv() #22812

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5febb21
Make a parameter const* to utf8_to_bytes_new_pv
khwilliamson Dec 20, 2024
6aced14
doio.c Change a variable from char to U8
khwilliamson Dec 20, 2024
e8617dc
do_print(): Use bytes_to_utf8_free_me, utf8_to_bytes_new_pv
khwilliamson Dec 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 16 additions & 22 deletions doio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2214,36 +2214,30 @@ 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);
U8 *tmpbuf = NULL;
tonycoz marked this conversation as resolved.
Show resolved Hide resolved
const U8 *tmps = (U8 *) SvPV_const(sv, len);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


/* If 'tmps' doesn't need converting, this will remain NULL and
* Safefree(free_me) will do nothing; Otherwise it points to the newly
* 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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per my comments on the other commits, this should not be const.

bool happy = TRUE;

if (PerlIO_isutf8(fp)) { /* If the stream is utf8 ... */
if (!SvUTF8(sv)) { /* Convert to utf8 if necessary */
/* We don't modify the original scalar. */
tmpbuf = bytes_to_utf8((const U8*) tmps, &len);
tmps = (char *) tmpbuf;
/* This doesn't modify the original scalar. */
tmps = bytes_to_utf8_free_me(tmps, &len, &free_me);
}
else if (ckWARN4_d(WARN_UTF8, WARN_SURROGATE, WARN_NON_UNICODE, WARN_NONCHAR)) {
(void) check_utf8_print((const U8*) tmps, len);
(void) check_utf8_print(tmps, len);
}
} /* else stream isn't utf8 */
else if (DO_UTF8(sv)) { /* But if is utf8 internally, attempt to
convert to bytes */
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_new_pv(&tmps, &len, &free_me)) {
/* Non-utf8 output stream, but string only representable in
utf8 */
Perl_ck_warner_d(aTHX_ packWARN(WARN_UTF8),
"Wide character in %s",
PL_op ? OP_DESC(PL_op) : "print"
Expand All @@ -2259,9 +2253,9 @@ 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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PerlIO_write() takes a const void * for the buffer, you don't need the cast.

happy = FALSE;
Safefree(tmpbuf);
Safefree(free_me);
return happy ? !PerlIO_error(fp) : FALSE;
}
}
Expand Down
2 changes: 1 addition & 1 deletion embed.fnc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Admp |bool |utf8_to_bytes_overwrite \
|NN U8 **s_ptr \
|NN STRLEN *lenp
Expand Down
2 changes: 1 addition & 1 deletion proto.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion utf8.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1344,7 +1344,7 @@ typedef enum {
Perl_utf8_to_bytes_(aTHX_ s, l, INT2PTR(U8 **, 1), \
PL_utf8_to_bytes_overwrite)
#define Perl_utf8_to_bytes_new_pv(mTHX, s, l, f) \
Perl_utf8_to_bytes_(aTHX_ (U8 **) s, l, f, \
Perl_utf8_to_bytes_(aTHX_ (U8 **) s, l, (U8 **) f, \
PL_utf8_to_bytes_new_memory)
#define Perl_utf8_to_bytes_temp_pv(mTHX, s, l) \
Perl_utf8_to_bytes_(aTHX_ (U8 **) s, l, INT2PTR(U8 **, 1), \
Expand Down
Loading