|
msg338232 - (view) |
Author: Stéphane Wirtel (matrixise) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
Date: 2019-03-24 06:02 |
I have closed the PR 12410. Feel free to submit another PR.
|
|
msg338731 - (view) |
Author: Ronald Oussoren (ronaldoussoren) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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
|
|
| Date |
User |
Action |
Args |
| 2022-04-11 14:59:12 | admin | set | github: 80528 |
| 2020-02-18 13:14:49 | miss-islington | set | nosy:
+ miss-islington messages:
+ msg362207
|
| 2019-04-04 10:34:55 | matrixise | set | messages:
+ msg339440 |
| 2019-04-04 10:31:15 | jdemeyer | set | pull_requests:
+ pull_request12612 |
| 2019-03-25 10:13:46 | ronaldoussoren | set | messages:
+ msg338794 |
| 2019-03-25 08:19:21 | serhiy.storchaka | set | messages:
+ msg338789 |
| 2019-03-24 22:47:35 | matrixise | set | messages:
+ msg338764 |
| 2019-03-24 21:40:22 | matrixise | set | messages:
+ msg338760 |
| 2019-03-24 21:37:15 | matrixise | set | nosy:
+ steve.dower messages:
+ msg338759
|
| 2019-03-24 21:33:25 | matrixise | set | messages:
+ msg338758 |
| 2019-03-24 21:27:17 | matrixise | set | pull_requests:
+ pull_request12478 |
| 2019-03-24 19:18:48 | matrixise | set | messages:
+ msg338745 |
| 2019-03-24 13:30:12 | matrixise | set | messages:
+ msg338732 |
| 2019-03-24 13:03:09 | ronaldoussoren | set | nosy:
+ ronaldoussoren messages:
+ msg338731
|
| 2019-03-24 06:02:56 | matrixise | set | messages:
+ msg338720 |
| 2019-03-20 10:25:30 | matrixise | set | messages:
+ msg338453 |
| 2019-03-19 19:13:35 | josh.r | set | messages:
+ msg338400 |
| 2019-03-19 15:39:02 | matrixise | set | title: Add the constant READWRITE for PyMemberDef -> Renaming the constants for the .flags of PyMemberDef |
| 2019-03-19 15:37:32 | matrixise | set | messages:
+ msg338366 |
| 2019-03-18 19:06:08 | josh.r | set | nosy:
+ josh.r messages:
+ msg338280
|
| 2019-03-18 17:05:06 | serhiy.storchaka | set | nosy:
+ serhiy.storchaka messages:
+ msg338261
|
| 2019-03-18 14:45:59 | matrixise | set | keywords:
+ patch stage: patch review pull_requests:
+ pull_request12364 |
| 2019-03-18 14:42:30 | matrixise | create | |