This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Renaming the constants for the .flags of PyMemberDef
Type: Stage: patch review
Components: Interpreter Core Versions: Python 3.8
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: josh.r, matrixise, miss-islington, ronaldoussoren, serhiy.storchaka, steve.dower
Priority: normal Keywords: patch

Created on 2019-03-18 14:42 by matrixise, last changed 2022-04-11 14:59 by admin.

Pull Requests
URL Status Linked Edit
PR 12410 closed matrixise, 2019-03-18 14:45
PR 12527 closed matrixise, 2019-03-24 21:27
PR 12684 merged jdemeyer, 2019-04-04 10:31
Messages (18)
msg338232 - (view) Author: Stéphane Wirtel (matrixise) * (Python committer) Date: 2019-03-18 14:42
When we define some members with PyMemberDef, we have to specify the flag for read-write or read-only. 

static PyMemberDef members[] = {
    {"name", T_OBJECT, offsetof(MyObject, name), 0, "Name object"},
    {NULL}  // Sentinel
};

For a newcomer, when you read the doc, you don't know the meaning of `0` and you want to know, of course you read the code and sometimes you can find READONLY or `0`.

I would like to add a new constant for `0` and name it `READWRITE`.

static PyMemberDef members[] = {
    {"name", T_OBJECT, offsetof(MyObject, name), READWRITE, "Name object"},
    {NULL}  // Sentinel
};
msg338261 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-03-18 17:05
It does not follow the convention for names in the C API. All public names should have prefix Py or PY, all private user visible names should have prefix _Py or _PY.

Some existing names do not follow that convention, but we should fix this and do not add new exceptions.
msg338280 - (view) Author: Josh Rosenberg (josh.r) * (Python triager) Date: 2019-03-18 19:06
Serhiy: Problem is that READONLY already exists, so PY_READWRITE would be inconsistent.

Given currently READONLY is just defined as:

#define READONLY 1

I suppose a solution to maintain consistency (of a sort) would be to add the definitions:

#define PY_READWRITE 0
#define PY_READONLY 1

leaving READONLY defined as well for backwards compatibility.

Names chosen are public names, since I'm pretty sure READONLY is considered part of the public API, given that PyMemberDef and its fields definitely are, and it would be impossible to use the flags field correctly if READONLY wasn't part of the public API.
msg338366 - (view) Author: Stéphane Wirtel (matrixise) * (Python committer) Date: 2019-03-19 15:37
@Serhiy

I have updated my branch with your recommendation.

1. rename READONLY, etc... to PY_READONLY. 
Why the PY_ prefix, because there was a renaming for WRITE_RESTRICTED to PY_WRITE_RESTRICTED. in that case, I wanted to keep the same change.

2. Updated the documentation
2.1. Add a "former constant" column in newtypes.rst
3. Update all the references in the code.
4. Replace the 0 by PY_READWRITE.

Voilà,

If you have another suggestion, tell me.

Have a nice day,
msg338400 - (view) Author: Josh Rosenberg (josh.r) * (Python triager) Date: 2019-03-19 19:13
Pretty sure you can't actually get rid of READONLY; it's part of the public API. Adding PY_READONLY to match PY_READWRITE is fine, but unlike WRITE_RESTRICTED/PY_WRITE_RESTRICTED (which isn't used at all in Py3,  has been non-functional since 2.3, and caused compilation errors on Visual Studio; more details on #36355), READONLY is commonly used by third party C extensions, doesn't have any known conflicts with compiler headers, and can't be removed.

Go ahead and add PY_READONLY, and change the documentation to prefer it, but:

#define READONLY 1

needs to stick around in the header indefinitely.
msg338453 - (view) Author: Stéphane Wirtel (matrixise) * (Python committer) Date: 2019-03-20 10:25
Hi @Josh,

In the PR, I don't remove READONLY. READONLY becomes an alias of
PY_READONLY.

I think I should split my PR in two PRs

1. Improve the current documentation and the code
2. Change the code in the Modules/ with the new macros.

@Serhyi what is your suggestion?

Thank you
msg338720 - (view) Author: Stéphane Wirtel (matrixise) * (Python committer) Date: 2019-03-24 06:02
I have closed the PR 12410. Feel free to submit another PR.
msg338731 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2019-03-24 13:03
Switches from #define's to an enum would allow explictly deprecating the old name (at least with clang and probably with GCC as well):

clang -c -Wall t.c
t.c:12:10: warning: 'READONLY' is deprecated: use PY_READONLY [-Wdeprecated-declarations]
        int i = READONLY;
                ^
t.c:7:26: note: 'READONLY' has been explicitly marked deprecated here
        READONLY __attribute__((deprecated("use PY_READONLY"))) = PY_READONLY
                                ^
1 warning generated.


For this source code:

#include <stdio.h>

enum {
	PY_READWRITE = 0,
	PY_READONLY = 1,

	READONLY __attribute__((deprecated("use PY_READONLY"))) = PY_READONLY
};

int main(void)
{
	int i = READONLY;

	printf("%d\n", i);
	return 0;
}

I'm not sure if it worthwhile switch to an enum here, the CPython source code isn't consistent in using enums for constants like this.
msg338732 - (view) Author: Stéphane Wirtel (matrixise) * (Python committer) Date: 2019-03-24 13:30
Pretty good.

In fact I wanted to replace the #define by const int and try to find a solution with gcc for a déprécation warning, but I was not sure about a such solution.

Your proposal is really interesting and open new solutions.

Thank you
msg338745 - (view) Author: Stéphane Wirtel (matrixise) * (Python committer) Date: 2019-03-24 19:18
Hi Ronald,

I have checked with gcc and clang on my computer (fedora 29)

This flag is defined in CLANG for the GNU and C++11 standard

See this table:

https://clang.llvm.org/docs/AttributeReference.html#deprecated

We could integrate it because the unused flag is also defined in GNU and
C++11 standard.

https://clang.llvm.org/docs/AttributeReference.html#maybe-unused-unused

For gcc, there is this reference

https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#Common-Variable-Attributes

### CLANG
LC_ALL=C clang test.c -o test
test.c:14:13: warning: 'READONLY' is deprecated: use PY_READONLY
[-Wdeprecated-declarations]
    int i = READONLY;
            ^
test.c:8:27: note: 'READONLY' has been explicitly marked deprecated here
    READONLY __attribute((deprecated("use PY_READONLY"))) = PY_READONLY,
                          ^
1 warning generated.

LC_ALL=C clang --version test.c -o test
clang version 7.0.1 (Fedora 7.0.1-6.fc29)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

### GCC
LC_ALL=C gcc --version test.c -o test
gcc (GCC) 8.3.1 20190223 (Red Hat 8.3.1-2)
Copyright (C) 2018 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

LC_ALL=C gcc test.c -o test
test.c: In function 'main':
test.c:14:5: warning: 'READONLY' is deprecated: use PY_READONLY
[-Wdeprecated-declarations]
     int i = READONLY;
     ^~~
test.c:8:5: note: declared here
     READONLY __attribute((deprecated("use PY_READONLY"))) = PY_READONLY,
     ^~~~~~~~

But I need to know the required version of gcc and clang, in this case,
we need to know if these attributes are supported by the compiler, but
normally this is the case.

Maybe, I should ask on the python-dev mailing list.

Thank you for your advice,
msg338758 - (view) Author: Stéphane Wirtel (matrixise) * (Python committer) Date: 2019-03-24 21:33
Ronald,

Please, could you check this PR and give me your advice,
For the moment, it's just re-declaration of the current constants with
the PY_ prefix and with an enumeration and the __attribute__(deprecated)

The PR is in Draft mode and I will update it with your remarks and the
feedback from the Python-dev ML

PR: https://github.com/python/cpython/pull/12527

There is a check list for me.

https://github.com/python/cpython/pull/12527#issuecomment-476002321
msg338759 - (view) Author: Stéphane Wirtel (matrixise) * (Python committer) Date: 2019-03-24 21:37
@steve.dower

Can I use this https://docs.microsoft.com/en-us/cpp/cpp/deprecated-cpp?view=vs-2017 for the deprecation?

I am going to try with my PR but I would like to have your approbation if it's the right solution or not.

Thank you
msg338760 - (view) Author: Stéphane Wirtel (matrixise) * (Python committer) Date: 2019-03-24 21:40
or with this feature of MSVS https://docs.microsoft.com/en-us/cpp/preprocessor/deprecated-c-cpp?view=vs-2017
msg338764 - (view) Author: Stéphane Wirtel (matrixise) * (Python committer) Date: 2019-03-24 22:47
With the help of Victor, I don't need to implement __attribute__((deprecated)), this one is already defined in pyport.h

So, I have updated my PR.

Next step, update all the references of READONLY, ..., by the new constants.
msg338789 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-03-25 08:19
I am against deprecating READONLY. This will break virtually every third-party extension that use it. Many projects are strong about compiler warning and will need to rewrite the code that worked for years.

I think that we can add the deprecation warning only after the last version that do not have PY_READONLY (3.7) will be no longer supported. I.e. in 3.11 or something around.

Also I am not sure about using enums for flags. Would not this cause problems on C++?

Since this is an extending of the C API, it would be better to discuss the necessary of adding PY_READWRITE on Python-Dev.
msg338794 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2019-03-25 10:13
A discussion on the use of enum and deprecation warnings might be useful, although there is a significant risk on bike shedding.

I agree that formally deprecating READONLY can lead to uglier code in extension that use the value and need to support anything beyond the bleeding edge (although the complication isn't that bad: just add a conditional definition of PY_READONLY).  I'm personally not to worried about this, and generally prefer being more aggressive with adding deprecation warnings.


W.r.t. C++ and enums: that should be ok, especially when using anonymous enums that are basically only used to name constants.  

Bitmask with enums can be more problematic when using named enums that are also used for variable definitions because C++ compilers are allowed to use the minimal variable size that can represent all defined values (that's also a problem for the ABI), but it is also possible to use scoped enums to specify the size of values.
msg339440 - (view) Author: Stéphane Wirtel (matrixise) * (Python committer) Date: 2019-04-04 10:34
For that, where did you see the macro was not used? maybe by an external
library.
msg362207 - (view) Author: miss-islington (miss-islington) Date: 2020-02-18 13:14
New changeset 24bba8cf5b8db25c19bcd1d94e8e356874d1c723 by Jeroen Demeyer in branch 'master':
bpo-36347: stop using RESTRICTED constants (GH-12684)
https://github.com/python/cpython/commit/24bba8cf5b8db25c19bcd1d94e8e356874d1c723
History
Date User Action Args
2022-04-11 14:59:12adminsetgithub: 80528
2020-02-18 13:14:49miss-islingtonsetnosy: + miss-islington
messages: + msg362207
2019-04-04 10:34:55matrixisesetmessages: + msg339440
2019-04-04 10:31:15jdemeyersetpull_requests: + pull_request12612
2019-03-25 10:13:46ronaldoussorensetmessages: + msg338794
2019-03-25 08:19:21serhiy.storchakasetmessages: + msg338789
2019-03-24 22:47:35matrixisesetmessages: + msg338764
2019-03-24 21:40:22matrixisesetmessages: + msg338760
2019-03-24 21:37:15matrixisesetnosy: + steve.dower
messages: + msg338759
2019-03-24 21:33:25matrixisesetmessages: + msg338758
2019-03-24 21:27:17matrixisesetpull_requests: + pull_request12478
2019-03-24 19:18:48matrixisesetmessages: + msg338745
2019-03-24 13:30:12matrixisesetmessages: + msg338732
2019-03-24 13:03:09ronaldoussorensetnosy: + ronaldoussoren
messages: + msg338731
2019-03-24 06:02:56matrixisesetmessages: + msg338720
2019-03-20 10:25:30matrixisesetmessages: + msg338453
2019-03-19 19:13:35josh.rsetmessages: + msg338400
2019-03-19 15:39:02matrixisesettitle: Add the constant READWRITE for PyMemberDef -> Renaming the constants for the .flags of PyMemberDef
2019-03-19 15:37:32matrixisesetmessages: + msg338366
2019-03-18 19:06:08josh.rsetnosy: + josh.r
messages: + msg338280
2019-03-18 17:05:06serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg338261
2019-03-18 14:45:59matrixisesetkeywords: + patch
stage: patch review
pull_requests: + pull_request12364
2019-03-18 14:42:30matrixisecreate