Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(261604)

#10156: Initialization of globals in unicodeobject.c

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 9 months ago by stefan
Modified:
6 years, 6 months ago
Reviewers:
storchaka
CC:
lemburg, Georg, amaury.forgeotdarc, Nick Coghlan, AntoinePitrou, haypo, stutzbach, Arfrever, skrah, devnull_psf.upfronthosting.co.za, storchaka, kushou, gregular_gmail.com
Visibility:
Public.

Patch Set 1 #

Patch Set 2 #

Patch Set 3 #

Patch Set 4 #

Patch Set 5 #

Patch Set 6 #

Total comments: 17

Patch Set 7 #

Total comments: 5

Patch Set 8 #

Patch Set 9 #

Patch Set 10 #

Patch Set 11 #

Patch Set 12 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Objects/unicodeobject.c View 1 2 3 4 5 6 7 8 9 10 11 34 chunks +87 lines, -114 lines 0 comments Download

Messages

Total messages: 6
stefan-usenet_bytereef.org
I think Serhiy's patch is the right approach before fixing things fundamentally in PEP 432. ...
6 years, 6 months ago #1
storchaka_gmail.com
http://bugs.python.org/review/10156/diff/7027/Objects/unicodeobject.c File Objects/unicodeobject.c (right): http://bugs.python.org/review/10156/diff/7027/Objects/unicodeobject.c#newcode179 Objects/unicodeobject.c:179: #define _Py_INCREF_UNICODE_EMPTY() do { \ On 2013/01/24 19:55:03, skrah ...
6 years, 6 months ago #2
storchaka_gmail.com
http://bugs.python.org/review/10156/diff/7027/Objects/unicodeobject.c File Objects/unicodeobject.c (right): http://bugs.python.org/review/10156/diff/7027/Objects/unicodeobject.c#newcode424 Objects/unicodeobject.c:424: Py_DECREF(unicode); On 2013/01/24 20:52:45, storchaka wrote: > It doesn't ...
6 years, 6 months ago #3
stefan-usenet_bytereef.org
I've a couple of comments. If you can address these, the 2.7 patch looks good ...
6 years, 6 months ago #4
stefan-usenet_bytereef.org
Attaching the comments to the old 3.4 patch, since there's no review link for the ...
6 years, 6 months ago #5
storchaka_gmail.com
6 years, 6 months ago #6
http://bugs.python.org/review/10156/diff/7027/Objects/unicodeobject.c
File Objects/unicodeobject.c (right):

http://bugs.python.org/review/10156/diff/7027/Objects/unicodeobject.c#newcode189
Objects/unicodeobject.c:189: #define _Py_RETURN_UNICODE_EMPTY()  do {           
    \
On 2013/01/25 16:31:55, skrah wrote:
> The first macro still is
> 
>   #define _Py_INCREF_UNICODE_EMPTY() do {
> 
> in the latest 3.4 patch.

Done.

http://bugs.python.org/review/10156/diff/7027/Objects/unicodeobject.c#newcode424
Objects/unicodeobject.c:424: Py_DECREF(unicode);
On 2013/01/25 16:31:55, skrah wrote:
> 
> Actually the macro creates unicode_empty with refcount 2 already. If you also
> change _Py_UnicodeInit to do the same, then we're (extra) safe.

This is not needed. The macro creates unicode_empty with refcount 2 because
there are two non-borrow references -- unicode_empty itself and result. When
result will be deleted, the refcount will be decreased to 1. _Py_Unicode_Init no
needed a non-borrow reference to unicode_empty.

http://bugs.python.org/review/10156/diff/7182/Objects/unicodeobject.c
File Objects/unicodeobject.c (right):

http://bugs.python.org/review/10156/diff/7182/Objects/unicodeobject.c#newcode86
Objects/unicodeobject.c:86: not be used before calling that API.
On 2013/01/25 13:13:19, skrah wrote:
> This comment is now stale. Perhaps something like:
> 
> "NOTE: In the interpreter's initialization phase, some globals are currently
>        initialized dynamically as needed. In the process Unicode objects may
>        be created before the Unicode type is ready."

Done.

http://bugs.python.org/review/10156/diff/7182/Objects/unicodeobject.c#newcode...
Objects/unicodeobject.c:8871: unicode_empty = _PyUnicode_New(0);
On 2013/01/25 13:13:19, skrah wrote:
> There's a refleak if unicode_empty has already been initialized via the
macros.
> 
> Also, the macros create unicode_empty with a refcount of 2. If unicode_empty
> happens to be created at this place, I'd prefer to have the same refcount so
we
> have some kind of invariant. 

Done.
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7+