Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replacing char* with const char* #45300

Closed
ext mannequin opened this issue Aug 12, 2007 · 16 comments
Closed

Replacing char* with const char* #45300

ext mannequin opened this issue Aug 12, 2007 · 16 comments
Assignees
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@ext
Copy link
Mannequin

ext mannequin commented Aug 12, 2007

BPO 1772673
Nosy @tim-one, @brettcannon, @amauryfa, @ncoghlan, @abalkin, @pitrou, @duggelz, @ned-deily, @ericsnowcurrently, @serhiy-storchaka
Dependencies
  • bpo-9369: const char* for PyObject_CallMethod and PyObject_CallFunction
  • bpo-1699259: replacing char* with const char* in sysmodule.c/.h
  • Files
  • const_char.patch: Patch to replace char* with const char* in a few functions
  • const_char_2.patch
  • const_char_3.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/serhiy-storchaka'
    closed_at = <Date 2013-10-19.18:05:11.709>
    created_at = <Date 2007-08-12.16:43:07.000>
    labels = ['interpreter-core', 'type-feature']
    title = 'Replacing char* with const char*'
    updated_at = <Date 2014-09-28.08:29:45.063>
    user = 'https://bugs.python.org/ext'

    bugs.python.org fields:

    activity = <Date 2014-09-28.08:29:45.063>
    actor = 'python-dev'
    assignee = 'serhiy.storchaka'
    closed = True
    closed_date = <Date 2013-10-19.18:05:11.709>
    closer = 'serhiy.storchaka'
    components = ['Interpreter Core']
    creation = <Date 2007-08-12.16:43:07.000>
    creator = 'ext-'
    dependencies = ['9369', '1699259']
    files = ['8163', '30440', '32124']
    hgrepos = []
    issue_num = 1772673
    keywords = ['patch']
    message_count = 16.0
    messages = ['53019', '58709', '58712', '62954', '189130', '189178', '189181', '189182', '189210', '190239', '190445', '199948', '200458', '200469', '200473', '227753']
    nosy_count = 13.0
    nosy_names = ['tim.peters', 'brett.cannon', 'amaury.forgeotdarc', 'ncoghlan', 'belopolsky', 'gustavo', 'pitrou', 'dgreiman', 'ext-', 'ned.deily', 'python-dev', 'eric.snow', 'serhiy.storchaka']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue1772673'
    versions = ['Python 3.4']

    @ext
    Copy link
    Mannequin Author

    ext mannequin commented Aug 12, 2007

    Many functions use char* where const char* should be used. I've changed this in a few function prototypes which I use while embedding python due to numerous warnings about deprecated casting from string constants to char*.

    @ext ext mannequin added interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Aug 12, 2007
    @gustavo
    Copy link
    Mannequin

    gustavo mannequin commented Dec 17, 2007

    I was about to submit the very same patch :-)
    I missed PyErr_NewException, but you missed the PyMapping_* methods ;)

    Please look into this matter. GCC 4.2 has arrived and it has a new
    warning. The code:

         char* kwlist[] = { "target", "encoding", NULL };

    now gives a warning like:

    "warning: deprecated conversion from string constant to ‘char*’"

    at least when compiling in C++ mode...

    This means that people have to declare it as "const char* kwlist", which
    will then trigger another warning when calling
    PyArg_ParseTupleAndKeywords with such a variable, because the prototype is:

    PyAPI_FUNC(int) PyArg_ParseTupleAndKeywords(PyObject *, PyObject *,
                                                      const char *, char **,
    ...);

    The "char **" should be "const char **". ext-, any chance you could fix
    that too?

    @amauryfa
    Copy link
    Member

    There was once a rather long discussion about "const" in
    PyArg_ParseTupleAndKeywords:
    http://mail.python.org/pipermail/python-dev/2006-February/060689.html

    If you don't read it to the end, here is the conclusion:
    """
    It sounds like the right answer for Python is to change the signature
    of PyArg_ParseTupleAndKeywords() back. We'll fix it when C fixes its
    const rules <wink>.
    """

    "const char*" was accepted without opposition, though.

    @tiran tiran added type-feature A feature request or enhancement labels Jan 6, 2008
    @abalkin
    Copy link
    Member

    abalkin commented Feb 25, 2008

    Douglas Greiman submitted many similar changes with his bpo-2135 patch.
    His patch also amends documentation, which is missing here.

    I am adding dgreiman to the nosy list here to avoid duplication of
    effort.

    I am -0 on the idea.

    @elibendersky
    Copy link
    Mannequin

    elibendersky mannequin commented May 13, 2013

    For external APIs visible to user code, this can cause some compatibility problems and users may need to at the very least re-compile this code. So -1 here.

    For new external APIs, having const wherever appropriate should be considered on a case by case basis.

    For internal code this is a good idea, but I wouldn't go mass-slashing all the code base now replacing char* by const char*. Presenting some examples where this makes sense in particular would be interesting though. +0

    @pitrou
    Copy link
    Member

    pitrou commented May 13, 2013

    For external APIs visible to user code, this can cause some
    compatibility problems and users may need to at the very least
    re-compile this code.

    Can you explain exactly which compatibility problems this would cause?

    Actually, the point is precisely to make life easier for third-party code, since "const char *" is more accepting than "char *".

    @elibendersky
    Copy link
    Mannequin

    elibendersky mannequin commented May 13, 2013

    Antoine, I was referring to the discussion linked earlier (http://mail.python.org/pipermail/python-dev/2006-February/060689.html). Users of the C API needed to recompile their code and also add preprocessor hacks to make things compatible with C and C++, AFAIU.

    @pitrou
    Copy link
    Member

    pitrou commented May 13, 2013

    Antoine, I was referring to the discussion linked earlier
    (http://mail.python.org/pipermail/python-dev/2006-February/060689.html).

    Ok, I've read it through. The problem is specifically with
    pointers-to-pointers:
    http://mail.python.org/pipermail/python-dev/2006-February/060849.html

    And indeed, PyArg_ParseTupleAndKeywords() got its "const char **"
    argument changed back to "char **". But the other consts have stayed
    (such as the "const char *" argument to the same
    PyArg_ParseTupleAndKeywords()).

    So it looks like the patch, in essence, is legit.
    (of course, in practice, it probably won't apply cleanly, since it's
    very old; and perhaps it's incomplete too)

    @serhiy-storchaka
    Copy link
    Member

    In C++ we may overload functions for backward compatibility:

    PyAPI_FUNC(int) PyArg_ParseTupleAndKeywords(PyObject *, PyObject *,
                            const char *, const char * const *, ...);
    PyAPI_FUNC(int) PyArg_VaParseTupleAndKeywords(PyObject *, PyObject *,
                            const char *, const char * const *, va_list va);
    ...
    #ifdef __cplusplus
    inline int
    PyArg_ParseTupleAndKeywords(PyObject *args, PyObject *keywords,
                            const char *format, char * const *kwlist, ...)
    {
        int retval;
        va_list va;
        va_start(va, kwlist);
        retval = PyArg_ParseTupleAndKeywords(args, keywords, format,
                            (const char * const *)kwlist, va_start);
        va_end(va);
        return retval;
    }
    #endif

    @serhiy-storchaka
    Copy link
    Member

    See also bpo-9369.

    @serhiy-storchaka
    Copy link
    Member

    The patch is outdated. Some function prototypes were changed in bpo-9369, some were changed before.

    Here is a new patch. It adds const qualifier to following public functions:
    PyObject_DelAttrString, PyObject_HasAttrString, PyObject_GetAttrString, PyObject_SetAttrString, PyObject_DelItemString, PyMapping_DelItemString, PyMapping_HasKeyString, PyMapping_GetItemString, PyMapping_SetItemString, PyFile_FromFd, PyImport_ExecCodeModule, PyImport_ExecCodeModuleEx, PyImport_ExecCodeModuleWithPathnames, PyImport_ImportFrozenModule, PyLong_FromString, PyOS_strtoul, PyOS_strtol, PyOS_Readline, PyMarshal_ReadObjectFromString, PyParser_ParseFile, PyParser_ParseFileFlags, PyParser_ParseFileFlagsEx, PyTokenizer_FromFile. It also changes prototypes of some internal functions and structures and fixes documentation for PyDict_DelItemString. Some of functions already were documented with const qualifier.

    @serhiy-storchaka
    Copy link
    Member

    The patch synchronized with tip.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 19, 2013

    New changeset 3055e61f1e66 by Serhiy Storchaka in branch 'default':
    Issue bpo-1772673: The type of char* arguments now changed to const char*.
    http://hg.python.org/cpython/rev/3055e61f1e66

    @ned-deily
    Copy link
    Member

    Compile errors on OS X 10.8 with clang:

    cc -Wno-unused-result -Werror=declaration-after-statement -g -O0 -Wall -Wstrict-prototypes     -I. -IInclude -I../../source/Include    -DPy_BUILD_CORE  -c ../../source/Modules/posixmodule.c -o Modules/posixmodule.o
    In file included from ../../source/Modules/posixmodule.c:5959:
    /usr/include/util.h:94:5: error: conflicting types for 'openpty'
    int     openpty(int *, int *, char *, struct termios *,
            ^
    ../../source/Include/pyport.h:676:12: note: previous declaration is here
    extern int openpty(int *, int *, char *,
               ^
    In file included from ../../source/Modules/posixmodule.c:5959:
    /usr/include/util.h:97:7: error: conflicting types for 'forkpty'
    pid_t   forkpty(int *, char *, struct termios *, struct winsize *);
            ^
    ../../source/Include/pyport.h:678:14: note: previous declaration is here
    extern pid_t forkpty(int *, char *,
                 ^
    2 errors generated.
    make: *** [Modules/posixmodule.o] Error 1

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 19, 2013

    New changeset 8bd69bd6e129 by Serhiy Storchaka in branch 'default':
    Restore prototypes for the 'openpty' and 'forkpty' on BSDI (broken in issue bpo-1772673).
    http://hg.python.org/cpython/rev/8bd69bd6e129

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 28, 2014

    New changeset 599a957038fa by Serhiy Storchaka in branch 'default':
    Removed redundant casts to char *.
    https://hg.python.org/cpython/rev/599a957038fa

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants