replacing char* with const char* in sysmodule.c/.h

[sebastinas]

BPO	1699259
Nosy	@abalkin, @tiran, @bitdancer
Files	sysmodule_const_char.diff: patch for Include/sysmodule.h and Python/sysmodule.c sysmodule_const_char_r67215.diff: updated 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/tiran'
closed_at = <Date 2011-06-05.07:12:09.285>
created_at = <Date 2007-04-12.13:55:10.000>
labels = ['interpreter-core']
title = 'replacing char* with const char* in sysmodule.c/.h'
updated_at = <Date 2011-06-05.07:12:09.284>
user = 'https://bugs.python.org/sebastinas'

bugs.python.org fields:

activity = <Date 2011-06-05.07:12:09.284>
actor = 'sebastinas'
assignee = 'christian.heimes'
closed = True
closed_date = <Date 2011-06-05.07:12:09.285>
closer = 'sebastinas'
components = ['Interpreter Core']
creation = <Date 2007-04-12.13:55:10.000>
creator = 'sebastinas'
dependencies = []
files = ['7939', '11999']
hgrepos = []
issue_num = 1699259
keywords = ['patch']
message_count = 15.0
messages = ['52430', '75812', '75833', '75837', '75839', '75844', '78795', '92943', '110578', '110604', '110618', '119027', '119031', '119032', '137685']
nosy_count = 6.0
nosy_names = ['jhylton', 'belopolsky', 'christian.heimes', 'sebastinas', 'eckhardt', 'r.david.murray']
pr_nums = []
priority = 'normal'
resolution = 'out of date'
stage = 'patch review'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue1699259'
versions = ['Python 2.7']

[sebastinas]

I'm embedding Python in a C++ application and found myself casting const char* to char* quite often when using functions like PySys_SetObject and PySys_GetObject.

I attached a patch replacing the char* arguments of the above mentioned functions, PySys_GetFile and PySys_AddWarnOption with const char*.

[abalkin]

The patch no longer applies to trunk, but that would be trivial to fix.

Changes like these have been accepted with little resistance in the past
(see e.g. bpo-651362), so I don't see why this patch has been pending
for so long. (In general, I am -0 on adding const modifiers because
they tend to trigger an avalanche of changes, but arguably this is the
"right thing to do.")

A few comments:

• Documentation needs to be updated in Doc/c-api/sys.rst.

• Any reason why PySys_SetPath(char *) is left out?

• Same for PySys_SetArgv(int, char **)

• Similar changes are proposed in bpo-1772673. Maybe these two issues
  can be dealt with together.

[sebastinas]

At least a response, finally.

• Any reason why PySys_SetPath(char *) is left out?

I guess it I just missed it.

• Same for PySys_SetArgv(int, char **)

That one is non-trivial and requires some rewriting of PySys_SetArgv.
And I didn't have the time to look into it any further.

I attached an updated patch.

[abalkin]

The new patch looks fine to me. It applies and compiles without
warnings and the changes are now reflected in the docs. I guess
someone would need to write a NEWS entry because it is a public API
change, but otherwise I would say it should be applied.

"[U]nless 'C' has adopted overloading while I wasn't looking ... it's
hard to imagine what kind of problems you could introduce in `C' code
by changing char* to char const* that wouldn't be caught at
compile-time." (Dave Abrahams in C++-sig)

http://mail.python.org/pipermail/cplusplus-sig/2005-December/009562.html

On Thu, Nov 13, 2008 at 2:24 PM, Sebastian Ramacher
<report@bugs.python.org> wrote:
..

> * Same for PySys_SetArgv(int, char **)

That one is non-trivial and requires some rewriting of PySys_SetArgv.
And I didn't have the time to look into it any further.

I agree and I only tossed this one in to check if you are aware of the
char ** issues. I believe it was agreed at some point that changing
char ** to const char ** is not a good idea (see revision 42596
reverting such change), but I could not find a clear explanation of
the associated problems.

In any case, +1 from me in favor of applying this patch and I will try
to add Jeremy Hylton to the "nosy" list since he is the developer who
made similar API changes in r41638 .

[tiran]

Sorry, but it's too late to apply the patch. The issues don't count as
"critical" and it changes the API, too. Only critical and important bugs
are solved during the release candidate phase of 3.0. Python 2.6 is
already out.

I set the target version to 2.7 and 3.1.

[abalkin]

While technically this is an API change, in reality it is unlikely to
break anyone's code because you can always pass char * to a function
that expects const char* and the ABI does not change. (Also I cannot
think why anyone would want to use pointers to the affected functions,
which would be another potential breakage.)

In any case, I am not a big proponent of const correctness, but this
patch was forgotten for 1.5 years and
deferring it to 2.7 and 3.1 is virtually equivalent to closing with "won't fix".

Is it clear that this patch is not a candidate for minor releases -
2.5.3 or 2.6.1? As I explained, it is not *really* an API change. If
it is a backport candidate, I would see benefit in committing it
sooner and blocking in py3k merge until 3.0 is out.

On Thu, Nov 13, 2008 at 4:29 PM, Christian Heimes
<report@bugs.python.org> wrote:

Christian Heimes <lists@cheimes.de> added the comment:

Sorry, but it's too late to apply the patch. The issues don't count as
"critical" and it changes the API, too. Only critical and important bugs
are solved during the release candidate phase of 3.0. Python 2.6 is
already out.

I set the target version to 2.7 and 3.1.

----------
stage: -> patch review
versions: +Python 2.7, Python 3.1 -Python 2.6

---

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

---

[eckhardt]

Concerning the SetArgv( int, char**), that one will have to be changed
to a SetArgv( int, char const* const*), i.e. applying the const on both
levels. Otherwise, there is no implicit conversion between the two.

The reason is a bit complicated: if the function stored a
pointer-to-const in the array, it would become a pointer-to-nonconst
outside. Only if modification of the array is forbidden, changing the
element type to pointer-to-const is allowed.

Otherwise, I'm +1 on this kind of changes, because it makes immediately
obvious which functions modify a passed buffer and which don't.

[bitdancer]

See also bpo-6952, which seems to be a broader request of the same
nature. Moving the 3.1 target to 3.2, since 3.1 is out.

[BreamoreBoy]

Drop 2.7 as that's now gone. See also bpo-6952.

[abalkin]

2.7 is not gone. It has entered an extended maintenance period. During this period, 2.7.x releases will incorporate bug fixes but will not get any new features. This particular issue is arguably a bug. I think Christian should make a call on whether this is appropriate for 2.7.

[BreamoreBoy]

This is quite clearly marked as a feature request and hence is 3.2. If you wish to change it go ahead, but who's going to do the work?

[sebastinas]

Any news on that?

[abalkin]

On Mon, Oct 18, 2010 at 11:17 AM, Sebastian Ramacher
<report@bugs.python.org> wrote:
..

Any news on that?

Is this patch still relevant for 3.2? It looks like const has been
added when char* was changed to wchar_t* in the affected functions.
See r62178.

[abalkin]

In fact, it looks like const has been added in py3k as early as r57439.

I am resetting "versions" to 2.7, but I am -0 on backporting. I am also unselecting "type" because with "feature request" type this should be closed while some may argue that this is a bug.

[sebastinas]

Since the patches are not applicable to Py 3.x and Py 2.7 is stable I'm closing this bug.