Use named constants internally in the re module

[serhiy-storchaka]

BPO	22434
Nosy	@gvanrossum, @rhettinger, @terryjreedy, @pitrou, @ezio-melotti, @serhiy-storchaka
Files	re_named_consts.patch re_opcode_enums.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2014-11-09.19:42:48.370>
created_at = <Date 2014-09-17.16:09:48.492>
labels = ['expert-regex', 'type-feature']
title = 'Use named constants internally in the re module'
updated_at = <Date 2014-11-11.19:23:50.707>
user = 'https://github.com/serhiy-storchaka'

bugs.python.org fields:

activity = <Date 2014-11-11.19:23:50.707>
actor = 'serhiy.storchaka'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2014-11-09.19:42:48.370>
closer = 'serhiy.storchaka'
components = ['Regular Expressions']
creation = <Date 2014-09-17.16:09:48.492>
creator = 'serhiy.storchaka'
dependencies = []
files = ['36642', '36657']
hgrepos = []
issue_num = 22434
keywords = ['patch']
message_count = 15.0
messages = ['227008', '227010', '227035', '227049', '227051', '227059', '227083', '227214', '227218', '227220', '230865', '230878', '230896', '230941', '231043']
nosy_count = 9.0
nosy_names = ['gvanrossum', 'effbot', 'rhettinger', 'terry.reedy', 'pitrou', 'ezio.melotti', 'mrabarnett', 'python-dev', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue22434'
versions = ['Python 3.5']

Linked PRs

• gh-66624: Add a name to re._constants.MAXGROUPS constant #120667

[serhiy-storchaka]

Regular expression parser parses a pattern to a tree, marking nodes by string identifiers. Regular expression compiler converts this three into plain list of integers. Node's identifiers are transformed to sequential integers. Resulting list is not human readable. Proposed patch converts string constants in the sre_constants module to named integer constants. These constants doesn't need converting to integers, because they are already integers, and when printed they looks human-friendly. Now intermediate result of regular expression compiler looks much more readable.

Example.

>> import re, sre_compile, sre_parse
>> sre_compile._code(sre_parse.parse('[a-z_][a-z_0-9]+', re.I), re.I)

Before patch:

[17, 4, 0, 2, 2147483647, 16, 7, 27, 97, 122, 19, 95, 0, 29, 16, 1, 2147483647, 16, 11, 10, 0, 67043328, 2147483648, 134217726, 0, 0, 0, 0, 0, 1, 1]

After patch:

[INFO, 4, 0, 2, MAXREPEAT, IN_IGNORE, 7, RANGE, 97, 122, LITERAL, 95, FAILURE, REPEAT_ONE, 16, 1, MAXREPEAT, IN_IGNORE, 11, CHARSET, 0, 67043328, 2147483648, 134217726, 0, 0, 0, 0, FAILURE, SUCCESS, SUCCESS]

This patch also affects debugging output when regular expression is compiled with re.DEBUG (identifiers are uppercased and MAXREPEAT is displayed instead of 2147483647 in repeat statements).

Besides debugging output these changes are invisible for ordinal user. They are needed only for developing and debugging the re module itself. The patch doesn't affect performance and almost not affects memory consumption.

[gvanrossum]

Love it!

[serhiy-storchaka]

If there are no objections I'll commit the patch soon.

[gvanrossum]

Hm. Could you not use the new Enum class?

[serhiy-storchaka]

Answering Guido's question about the Enum class. No, it is not appropriate here. It has too cumbersome repr (<OPCODES.IN_IGNORE: 16> instead of IN_IGNORE). Enum function syntax can't by used because it enumerates values from 1. We need three Enum subclasses for three groups of constants, and fourth class for MAXREPEAT, and fifth base abstract class. To fit the Enum class to our need we need more boilerplate code than to implement minimal needed functionality from scratch. And I'm not sure that this will not create circular dependency.

[gvanrossum]

I think you are too casual in rejecting a standard approach over a custom
clever hack. Making the values enums gives them a standard interface that
goes beyond what you implemented, and just the fact that we can say "these
are IntEnum instances" specifies a lot more of the semantics than pointing
to your class hack.

It's easy to make a subclass of IntEnum that overrides __repr__ and
__str__. It's also easy enough to override the constructor to make the
values start at 0 (though it would be a nice feature if we could add a
keyword arg to the EnumMeta.__call__() definition to override the start
position). I don't really care that this is more code.

I don't see where there would be a circular dependency; the enum module
doesn't import the re module.

There is one thing that might be less convenient: defining an enum doesn't
automatically make the values globals. But wouldn't the code be better if
the values weren't globals?

Finally. You objected to adding __all__ in the code review. That too,
suggests a somewhat casual attitude. This code may be maintained by your
grandchildren. Give them something future-proof, please.

On Thu, Sep 18, 2014 at 9:14 AM, Serhiy Storchaka <report@bugs.python.org>
wrote:

Serhiy Storchaka added the comment:

Answering Guido's question about the Enum class. No, it is not appropriate
here. It has too cumbersome repr (<OPCODES.IN_IGNORE: 16> instead of
IN_IGNORE). Enum function syntax can't by used because it enumerates values
from 1. We need three Enum subclasses for three groups of constants, and
fourth class for MAXREPEAT, and fifth base abstract class. To fit the Enum
class to our need we need more boilerplate code than to implement minimal
needed functionality from scratch. And I'm not sure that this will not
create circular dependency.

----------

---

Python tracker <report@bugs.python.org>
<http://bugs.python.org/issue22434\>

---

[serhiy-storchaka]

Here is a patch using enums. I still think enums are superfluous here. Advanced the Enum class features (pickling, access by name, type checking) are not needed - these constants don't leaked in outer word.

I don't see where there would be a circular dependency; the enum module
doesn't import the re module.

Right now there is no circular dependency. But the enum module imports collections which imports a lot of other modules, some of which import other modules. In future some of indirectly imported module can import the re module.

There is one thing that might be less convenient: defining an enum doesn't
automatically make the values globals. But wouldn't the code be better if
the values weren't globals?

I afraid this will make the code of parser and compiler less readable and slower. In any case the sre_constants module itself is a namespace.

[pitrou]

I think I agree with Serhiy. Augmenting the import dependencies from the re module can have side effects. While the alternative (a custom Int subclass) isn't extremely pretty, it does the job.

NamedIntConstant should be private, though.

[gvanrossum]

OK, then if you want to review Serhiy's original patch that would be great.

On Sun, Sep 21, 2014 at 5:12 AM, Antoine Pitrou <report@bugs.python.org>
wrote:

Antoine Pitrou added the comment:

I think I agree with Serhiy. Augmenting the import dependencies from the
re module can have side effects. While the alternative (a custom Int
subclass) isn't extremely pretty, it does the job.

NamedIntConstant should be private, though.

----------

---

Python tracker <report@bugs.python.org>
<http://bugs.python.org/issue22434\>

---

[serhiy-storchaka]

NamedIntConstant should be private, though.

Agree. I'll add underscore to NamedIntConstant and makecode before committing
(as in enum patch).

[serhiy-storchaka]

Could you please make a review of any patch Antoine? This would help me to debug re engine. It doesn't matter which patch apply, with good chance all this will be changed before 3.5 release and may be not once.

[rhettinger]

I reviewed re_named_consts.patch and it looks great (I especially like the removal of superfluous OPCODES dictionary lookups and improved repr for the integer codes).

Since the op codes are singletons, you can use identity tests instead of
equality checks in sre_parse.py:

•  if op == "in":
  

+ if op == IN:

Also, I'll echo the suggestion to make NamedIntConstant private with a leading underscore.

Nice work.