classification
Title: replacing char* with const char* in sysmodule.c/.h
Type: Stage: patch review
Components: Interpreter Core Versions: Python 2.7
process
Status: closed Resolution: out of date
Dependencies: Superseder:
Assigned To: christian.heimes Nosy List: belopolsky, christian.heimes, eckhardt, jhylton, r.david.murray, sebastinas
Priority: normal Keywords: patch

Created on 2007-04-12 13:55 by sebastinas, last changed 2011-06-05 07:12 by sebastinas. This issue is now closed.

Files
File name Uploaded Description Edit
sysmodule_const_char.diff sebastinas, 2007-04-12 13:55 patch for Include/sysmodule.h and Python/sysmodule.c review
sysmodule_const_char_r67215.diff sebastinas, 2008-11-13 19:23 updated patch review
Messages (15)
msg52430 - (view) Author: Sebastian Ramacher (sebastinas) Date: 2007-04-12 13:55
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*.
msg75812 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2008-11-13 04:26
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. issue651362), 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 issue1772673.  Maybe these two issues 
can be dealt with together.
msg75833 - (view) Author: Sebastian Ramacher (sebastinas) Date: 2008-11-13 19:23
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.
msg75837 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2008-11-13 20:50
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 .
msg75839 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-11-13 21:29
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.
msg75844 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2008-11-13 23:45
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>
> _______________________________________
>
msg78795 - (view) Author: Ulrich Eckhardt (eckhardt) Date: 2009-01-02 14:03
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.
msg92943 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2009-09-21 14:08
See also issue 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.
msg110578 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2010-07-17 16:32
Drop 2.7 as that's now gone.  See also #6952.
msg110604 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-07-17 21:28
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.
msg110618 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2010-07-17 23:52
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?
msg119027 - (view) Author: Sebastian Ramacher (sebastinas) Date: 2010-10-18 15:17
Any news on that?
msg119031 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-10-18 15:34
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.
msg119032 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-10-18 15:44
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.
msg137685 - (view) Author: Sebastian Ramacher (sebastinas) Date: 2011-06-05 07:12
Since the patches are not applicable to Py 3.x and Py 2.7 is stable I'm closing this bug.
History
Date User Action Args
2011-06-05 07:12:09sebastinassetstatus: open -> closed
resolution: out of date
messages: + msg137685
2010-10-18 15:44:41belopolskysetversions: + Python 2.7, - Python 3.2
nosy: - BreamoreBoy

messages: + msg119032

type: enhancement ->
2010-10-18 15:34:13belopolskysetmessages: + msg119031
2010-10-18 15:17:15sebastinassetmessages: + msg119027
2010-07-17 23:52:30BreamoreBoysetmessages: + msg110618
2010-07-17 21:28:29belopolskysetmessages: + msg110604
2010-07-17 16:32:28BreamoreBoysetnosy: + BreamoreBoy

messages: + msg110578
versions: - Python 2.7
2009-09-21 14:08:01r.david.murraysetnosy: + r.david.murray

messages: + msg92943
versions: + Python 3.2, - Python 3.1
2009-05-15 01:51:24ajaksu2linkissue1772673 dependencies
2009-01-02 14:03:38eckhardtsetnosy: + eckhardt
messages: + msg78795
2008-11-13 23:45:50belopolskysetmessages: + msg75844
2008-11-13 21:29:35christian.heimessetstage: patch review
messages: + msg75839
versions: + Python 3.1, Python 2.7, - Python 2.6
2008-11-13 20:53:54belopolskysetnosy: + jhylton
2008-11-13 20:51:00belopolskysetmessages: + msg75837
2008-11-13 19:23:58sebastinassetfiles: + sysmodule_const_char_r67215.diff
messages: + msg75833
2008-11-13 04:26:03belopolskysetnosy: + belopolsky
messages: + msg75812
2008-01-06 11:58:13christian.heimessetassignee: christian.heimes
type: enhancement
nosy: + christian.heimes
versions: + Python 2.6
2007-04-12 13:55:10sebastinascreate