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.

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

gh-46236: Add missing PyUnicode_Append() doc #130531

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

rruuaanng
Copy link
Contributor

@rruuaanng rruuaanng commented Feb 25, 2025

@bedevere-app bedevere-app bot mentioned this pull request Feb 25, 2025
33 tasks
@rruuaanng
Copy link
Contributor Author

cc @encukou

@encukou
Copy link
Member

encukou commented Feb 26, 2025

The docs should explain the unusual reference counting behaviour -- the function is “reference-neutral”, as in PyUnicode_InternInPlace, but on error you lose the reference *p_left.

Would you mind documenting PyUnicode_AppendAndDel as well?

@rruuaanng
Copy link
Contributor Author

rruuaanng commented Feb 26, 2025

The docs should explain the unusual reference counting behaviour -- the function is “reference-neutral”, as in PyUnicode_InternInPlace, but on error you lose the reference *p_left.

Would you mind documenting PyUnicode_AppendAndDel as well?

Of course, I accept any reasonable request.

Edit

Oh, by the way, could you help me check off the tasks in the issue checklist? It seems they haven’t been updated in a while.

@encukou
Copy link
Member

encukou commented Feb 27, 2025

Oh, by the way, could you help me check off the tasks in the issue checklist? It seems they haven’t been updated in a while.

I'm checking them off when the docs are merged, but if there's one I missed let me know which one.

Append the string *right* to the end of *p_left*.
*p_left* must point to a :term:`strong reference` to a Unicode object.

On error, set *p_left* to ``NULL`` (*stealing* the reference) and set an exception.
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the "stealing the reference" part. It's stolen to be put where? The reference is just destroyed by Py_DECREF() (via Py_CLEAR()).

Nitpick: the assignment is not on "p_left" but "*p_left". You should write set *\*p_left*.

Copy link
Member

Choose a reason for hiding this comment

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

I suggested that part--I'd also be ok with "releasing the reference," but I'm worried that's less clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that Py_DECREF(left) exists only in the else block, so in general, the reference count of p_left should remain unchanged. I removed the ambiguous borrowed reference to avoid any potential misunderstandings.
(I hope I haven't misunderstood the code.)

    if (unicode_modifiable(left)
        && PyUnicode_CheckExact(right)
        && PyUnicode_KIND(right) <= PyUnicode_KIND(left)
        /* Don't resize for ascii += latin1. Convert ascii to latin1 requires
           to change the structure size, but characters are stored just after
           the structure, and so it requires to move all characters which is
           not so different than duplicating the string. */
        && !(PyUnicode_IS_ASCII(left) && !PyUnicode_IS_ASCII(right)))
    {
        /* append inplace */
        if (unicode_resize(p_left, new_len) != 0)
            goto error;

        /* copy 'right' into the newly allocated area of 'left' */
        _PyUnicode_FastCopyCharacters(*p_left, left_len, right, 0, right_len);
    }
    else {
        maxchar = PyUnicode_MAX_CHAR_VALUE(left);
        maxchar2 = PyUnicode_MAX_CHAR_VALUE(right);
        maxchar = Py_MAX(maxchar, maxchar2);

        /* Concat the two Unicode strings */
        res = PyUnicode_New(new_len, maxchar);
        if (res == NULL)
            goto error;
        _PyUnicode_FastCopyCharacters(res, 0, left, 0, left_len);
        _PyUnicode_FastCopyCharacters(res, left_len, right, 0, right_len);
        Py_DECREF(left);
        *p_left = res;
    }

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants