Issue 15092: Using enum PyUnicode_Kind (original) (raw)

Created on 2012-06-17 09:21 by serhiy.storchaka, last changed 2022-04-11 14:57 by admin. This issue is now closed.

Messages (6)

msg163043 - (view)

Author: Serhiy Storchaka (serhiy.storchaka) *

Date: 2012-06-17 09:20

Now a string kind declared in different places of code as enum PyUnicode_Kind, int or unsigned int. Working on the codecs optimization, I noticed that sometimes the use of enum PyUnicode_Kind gives a little advantage over the use of int's. Probably this hint allows compiler to better utilize the optimizer. The proposed patch replaces all string kind's declarations of local variables and private functions to enum PyUnicode_Kind. If this will not impact significantly on the performance, then at least the unification will enhance the readability and will allow to avoid some of the errors (missing switch cases).

msg163531 - (view)

Author: Terry J. Reedy (terry.reedy) *

Date: 2012-06-23 05:46

Since we have defined an enum, I think we should use it consistently. (Am I correct in thinking it was added after the initial patches.) So I did a sanity check of -/+ lines for perhaps unintended changes. The only things I found to comment on, mostly somewhat picky, would not trigger test failures.

===

•        int kind = PyUnicode_KIND(output);

•         kind = PyUnicode_KIND(output);

extra space added misaligns text (for humans)

===

• default: assert(0);

I presume this is standard elsewhere for switches and just got omitted here.

• }
• assert(0);
• return -1;

• default:

•    assert(0);

•    return -1;

• }

(two places)

Since assert(0) always fails, return can never happen (and was not added above. So I would think remove it. And I agree with putting it consistently under default: (which is a form of else) even if not required as it improves human readability.

===

• int kind_self;
• int kind_sub;

• enum PyUnicode_Kind kind_self;
• enum PyUnicode_Kind kind_sub; void *data_self; void *data_sub;

Several places elsewhere, multiple enum vars are defined on one line, so you could do the same here (and with void *vars?).

====

• int kind1, kind2, kind;

• enum PyUnicode_Kind kind1, kind2, kind;

msg163547 - (view)

Author: Serhiy Storchaka (serhiy.storchaka) *

Date: 2012-06-23 08:13

Since assert(0) always fails, return can never happen (and was not added above. So I would think remove it.

This will cause a compiler warning in non-debug mode.

Here is updated patch with all other comments taken into account.

msg165745 - (view)

Author: Serhiy Storchaka (serhiy.storchaka) *

Date: 2012-07-18 06:19

Please, can anyone do the review of this large but trivial patch?

msg165751 - (view)

Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) *

Date: 2012-07-18 10:33

I'd strongly prefer not having "enum" everywhere. "PyUnicode_Kind" alone is certainly possible, maybe with a typedef?

msg186250 - (view)

Author: STINNER Victor (vstinner) *

Date: 2013-04-07 21:58

Python 3.3 has been released. I'm not sure that we can change the type of PyUnicode_Kind because of the stable API. By the way, I already tried to write a similar patch (use enum). I expected better performances, but it did not change anything.

Can we close this issue?

History

Date

User

Action

Args

2022-04-11 14:57:31

admin

set

github: 59297

2013-04-07 22:19:36

serhiy.storchaka

set

status: open -> closed
resolution: rejected
stage: patch review -> resolved

2013-04-07 21:58:49

vstinner

set

messages: +

2012-07-18 10:33:30

amaury.forgeotdarc

set

nosy: + amaury.forgeotdarc
messages: +

2012-07-18 06:19:07

serhiy.storchaka

set

messages: +

2012-06-23 08:13:42

serhiy.storchaka

set

files: + enum_PyUnicode_Kind-2.patch

messages: +

2012-06-23 05:46:13

terry.reedy

set

nosy: + terry.reedy
messages: +

2012-06-17 10:22:01

pitrou

set

nosy: + vstinner

stage: patch review

2012-06-17 09:21:06

serhiy.storchaka

create