classification
Title: Derby: Convert the _sre module to use Argument Clinic
Type: enhancement Stage: resolved
Components: Argument Clinic, Extension Modules, Regular Expressions Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: 20141 20144 20161 20283 Superseder:
Assigned To: serhiy.storchaka Nosy List: ezio.melotti, larry, mrabarnett, python-dev, serhiy.storchaka, steve.dower
Priority: normal Keywords: patch

Created on 2014-01-06 16:04 by serhiy.storchaka, last changed 2015-05-04 12:47 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
sre_clinic.patch serhiy.storchaka, 2014-01-18 12:23 review
sre_clinic_2.patch serhiy.storchaka, 2014-03-20 09:30 review
sre_clinic_3.patch serhiy.storchaka, 2014-06-01 15:59 review
sre_clinic_4.patch serhiy.storchaka, 2014-12-01 14:09 review
sre_clinic_5.patch serhiy.storchaka, 2015-04-17 20:11 review
larry.fix.sre.build.on.windows.diff.1.txt larry, 2015-05-03 19:54 review
Messages (29)
msg207448 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-06 16:04
Here is preliminary patch which converts the _sre module to use Argument Clinic. 22 functions are converted.

There are thre problems:

* Issue20141. The code was manually edited after Argument Clinic (and should be edited again for next run of Argument Clinic).

* Issue20144. Literal 1000000000 is temporary used instead of PY_SSIZE_T_MAX.

* Pydoc for _sre.compile (and only for this function) returns:

    _sre.compile = <built-in function compile>
msg207450 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-06 16:17
Obviously we can't live with manually editing the output from Argument Clinic, so I'll get you a fix for O! today.

Maybe we could use a better literal value?  Like 2**31 - 1?

I don't understand the pydoc thing.  Can you elaborate?
msg207452 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-06 16:36
> Maybe we could use a better literal value?  Like 2**31 - 1?

This only a placeholder. It will be replaced by named constant in final patch. 
2**31 - 1 is not better here than any other arbitrary value.

> I don't understand the pydoc thing.  Can you elaborate?

Pydoc for _sre.compile doesn't output signature and docstring, but only 
mentioned above line. There is similar issue in audioop module (issue20133) 
for the ratecv function.
msg207594 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-07 20:53
Can you refresh the patch?  I think all the problems you cited are fixed, and also the comments Argument Clinic uses were all changed.  I'll review when you have a fresh patch.
msg207627 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-07 23:57
Here is refreshed patch.
msg207646 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-08 00:25
Serhiy: Assigning to you because you wrote a patch; if you don't want the issue, sorry, please undo it.
msg208397 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-18 12:23
Now all methods except Match.group() (which needs *args) use Argument Clinic.

Most important change is that first parameters of some Pattern methods are renamed from "pattern" or "source" to "string". This was obvious bug (issue20283). "string" conforms to the documentation and to the name of the Match.string attribute.
msg214194 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-03-20 09:30
Here is updated patch.
msg231939 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-12-01 14:09
Synchronized with the tip again.
msg241359 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-04-17 20:11
Updated to the tip.
msg242470 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-05-03 13:18
New changeset d5f78a855355 by Serhiy Storchaka in branch 'default':
Issue #20148: Converted the _sre module to Argument Clinic.
https://hg.python.org/cpython/rev/d5f78a855355
msg242502 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-05-03 19:37
This broke Windows builds because of unnecessary "static" qualifiers on the forward declarations at lines 1429, 2472 and 2715 (as discussed on #20323).

Removing "static" from these lines fixes the build.
msg242505 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2015-05-03 19:48
Removing "static" breaks GCC on Linux (gcc 4.9.2 on Ubuntu 15.04 x86-64):

./Modules/_sre.c:2780:20: error: static declaration of ‘pattern_methods’ follows non-static declaration
 static PyMethodDef pattern_methods[] = {
                    ^
./Modules/_sre.c:1429:13: note: previous declaration of ‘pattern_methods’ was here
 PyMethodDef pattern_methods[];
             ^

IMO MSVS is the one being unreasonable here.  But obviously we need to find some way of doing this that makes both compilers happy.
msg242506 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2015-05-03 19:54
Steve, does this patch fix the build for Windows?  By all rights it oughta.
msg242508 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-05-03 19:59
This line (1429 before patch, 1433 after patch) is still being troublesome:

static PyMethodDef pattern_methods[];
msg242509 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-05-03 20:00
But looks like it's unnecessary and just wasn't removed in the patch. Everything builds fine without it
msg242510 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2015-05-03 20:01
Sorry--you can simply remove that line, it's no longer needed.
msg242511 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-05-03 20:01
Does your patch just move type definition to the end of the file? I think this is only the way to make happy GCC and MS compiler.

Strange that no IRC bot complained about compilation failure.
msg242512 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2015-05-03 20:03
Yes, it moves the type declaration to the bottom of the file, and adds forward static declarations for the types to the top of the file.
msg242513 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-05-03 20:08
AFAIK the only buildbot with a nearly-up-to-date MSVC is still marked as unstable (http://buildbot.python.org/all/builders/AMD64%20Windows8%203.x). I keep meaning to get it promoted, but haven't quite gotten around to it yet...
msg242514 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2015-05-03 20:10
Oh, this is only happening on the *beta* compiler?  In that case, I genuinely suggest you file a bug.

We can still check in the workaround, but I really do think MSVS's behavior is wrong here.  (Why is it only for forward static declarations of arrays of unspecified size?)
msg242518 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-05-03 20:22
Then the patch LGTM. It is easier to make these changes by hand than make a review for moved code on Rietveld.

I'm not sure, but may be forward static declarations of arrays of unspecified size is C99-ism?
msg242519 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-05-03 20:32
It fails on VC10, 11, 12 and 14, so I doubt it's going to be changed. That said, it looks like the non-static forward definition may be some sort of extension, since it causes a redefinition error (mismatched storage class) on all versions when using /Za to disable extensions.

It could be a C99 feature ("tentative definitions" I think), as Serhiy suggests, but if so then it's very likely not going to be added between the RC (released last week) and final versions of the compiler.
msg242520 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-05-03 20:42
According to http://compgroups.net/comp.std.c/why-does-the-standard-prohibit-static-int-a/2569729, it's invalid in both C89 and C99, but some compilers accept it as an extension.

IMO we should avoid relying on compiler extensions, at least in the code files that are supposed to be portable.
msg242521 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2015-05-03 20:47
I agree.  I looked it up in the C99 standard.  6.9.2.2 says:

"If the declaration of an identifier for an object is a tentative definition and has internal linkage, the declared type shall not be an incomplete type."

And if you'd hurry up and bless my patch for Modules/_tkinter.c in bug #20168, I'd check this fix in ;-)
msg242522 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2015-05-03 20:48
(sorry, 6.9.2.3)
msg242529 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-05-03 21:44
Sorry, wasn't watching the other issue. :)
msg242531 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-05-03 21:49
New changeset 7a76c462c7f6 by Larry Hastings in branch 'default':
Fix Windows build breakage from checkins on Issues #20148 and #20168.
https://hg.python.org/cpython/rev/7a76c462c7f6
msg242532 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2015-05-03 21:53
Steve, please close this issue when you've confirmed it's now building correctly on Windows.
History
Date User Action Args
2015-05-04 12:47:41serhiy.storchakasetstatus: open -> closed
resolution: fixed
2015-05-03 21:53:52larrysetmessages: + msg242532
2015-05-03 21:49:51python-devsetmessages: + msg242531
2015-05-03 21:44:19steve.dowersetmessages: + msg242529
2015-05-03 20:48:32larrysetmessages: + msg242522
2015-05-03 20:47:32larrysetmessages: + msg242521
2015-05-03 20:42:11steve.dowersetmessages: + msg242520
2015-05-03 20:32:23steve.dowersetmessages: + msg242519
2015-05-03 20:22:23serhiy.storchakasetmessages: + msg242518
2015-05-03 20:10:48larrysetmessages: + msg242514
2015-05-03 20:08:58steve.dowersetmessages: + msg242513
2015-05-03 20:03:07larrysetmessages: + msg242512
2015-05-03 20:01:58serhiy.storchakasetmessages: + msg242511
2015-05-03 20:01:16larrysetmessages: + msg242510
2015-05-03 20:00:39steve.dowersetmessages: + msg242509
2015-05-03 19:59:56steve.dowersetmessages: + msg242508
2015-05-03 19:54:56larrysetfiles: + larry.fix.sre.build.on.windows.diff.1.txt

messages: + msg242506
2015-05-03 19:48:58larrysetmessages: + msg242505
2015-05-03 19:38:10steve.dowersetstatus: closed -> open
resolution: fixed -> (no value)
2015-05-03 19:37:43steve.dowersetnosy: + steve.dower
messages: + msg242502
2015-05-03 14:32:13serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2015-05-03 13:18:49python-devsetnosy: + python-dev
messages: + msg242470
2015-04-17 20:11:26serhiy.storchakasetfiles: + sre_clinic_5.patch

messages: + msg241359
2015-02-25 15:30:01serhiy.storchakasetcomponents: + Argument Clinic
2014-12-01 14:09:44serhiy.storchakasetfiles: + sre_clinic_4.patch

messages: + msg231939
2014-06-01 15:59:09serhiy.storchakasetfiles: + sre_clinic_3.patch
2014-04-29 05:30:45terry.reedyunlinkissue21054 dependencies
2014-04-29 05:30:09terry.reedylinkissue21054 dependencies
2014-03-20 09:30:28serhiy.storchakasetfiles: + sre_clinic_2.patch

messages: + msg214194
versions: + Python 3.5, - Python 3.4
2014-02-08 19:28:08serhiy.storchakasetdependencies: + Wrong keyword parameter name in regex pattern methods
2014-01-18 12:23:59serhiy.storchakasetstage: patch review
2014-01-18 12:23:33serhiy.storchakasetfiles: + sre_clinic.patch

messages: + msg208397
2014-01-18 12:09:21serhiy.storchakasetfiles: - sre_clinic.patch
2014-01-18 12:08:44serhiy.storchakasetfiles: - sre_clinic.patch
2014-01-08 00:33:08larrylinkissue20187 dependencies
2014-01-08 00:25:38larrysetassignee: serhiy.storchaka
messages: + msg207646
2014-01-07 23:57:19serhiy.storchakasetfiles: + sre_clinic.patch

messages: + msg207627
2014-01-07 20:53:45larrysetmessages: + msg207594
title: Convert the _sre module to use Argument Clinic -> Derby: Convert the _sre module to use Argument Clinic
2014-01-07 09:40:29serhiy.storchakasetdependencies: + inspect.signature fails on some functions which use Argument Clinic
2014-01-06 17:08:14serhiy.storchakasetdependencies: + Argument Clinic: broken support for 'O!', Argument Clinic doesn't support named constants as default values
2014-01-06 16:36:10serhiy.storchakasetmessages: + msg207452
2014-01-06 16:17:07larrysetmessages: + msg207450
2014-01-06 16:05:26serhiy.storchakasetfiles: + sre_clinic.patch
keywords: + patch
2014-01-06 16:04:28serhiy.storchakacreate