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

const arg for PyInt_FromString #46174

Closed
philipdumont mannequin opened this issue Jan 18, 2008 · 8 comments
Closed

const arg for PyInt_FromString #46174

philipdumont mannequin opened this issue Jan 18, 2008 · 8 comments
Labels
extension-modules C modules in the Modules dir type-feature A feature request or enhancement

Comments

@philipdumont
Copy link
Mannequin

philipdumont mannequin commented Jan 18, 2008

BPO 1866
Nosy @gvanrossum, @doerwalter, @ncoghlan
Files
  • Python-2.5.patch
  • autotest.out.after: Output of test run after patch
  • make_test_after: Output of 'make test' after 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 = None
    closed_at = <Date 2010-09-20.11:16:41.650>
    created_at = <Date 2008-01-18.18:46:08.534>
    labels = ['extension-modules', 'type-feature']
    title = 'const arg for PyInt_FromString'
    updated_at = <Date 2010-09-20.11:16:41.649>
    user = 'https://bugs.python.org/philipdumont'

    bugs.python.org fields:

    activity = <Date 2010-09-20.11:16:41.649>
    actor = 'ncoghlan'
    assignee = 'none'
    closed = True
    closed_date = <Date 2010-09-20.11:16:41.650>
    closer = 'ncoghlan'
    components = ['Extension Modules']
    creation = <Date 2008-01-18.18:46:08.534>
    creator = 'philipdumont'
    dependencies = []
    files = ['9265', '9266', '9417']
    hgrepos = []
    issue_num = 1866
    keywords = ['patch']
    message_count = 8.0
    messages = ['60108', '60117', '61531', '62312', '62321', '87734', '116916', '116924']
    nosy_count = 5.0
    nosy_names = ['gvanrossum', 'doerwalter', 'ncoghlan', 'philipdumont', 'BreamoreBoy']
    pr_nums = []
    priority = 'low'
    resolution = 'rejected'
    stage = None
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue1866'
    versions = ['Python 3.2']

    @philipdumont
    Copy link
    Mannequin Author

    philipdumont mannequin commented Jan 18, 2008

    In PyInt_FromString(), please change the type of the first
    arg from char * to const char *. That is, of course, if
    the function indeed does not write to the string. (I took
    a quick look at the code; it doesn't appear to.)

    If the function does modify the string, it should be
    documented.

    I'm getting a compiler error in some C++ code when I
    use a std::string::c_str() as the arg. Of course, I could
    cast away the const, but paranoid as I am, I'd like
    reassurance that I can do so with impunity. :-)

    @philipdumont philipdumont mannequin added extension-modules C modules in the Modules dir type-feature A feature request or enhancement labels Jan 18, 2008
    @gvanrossum
    Copy link
    Member

    So how about submitting a patch *and* building everything from scratch
    and running all the unit tests to make sure this doesn't break anything?

    @philipdumont
    Copy link
    Mannequin Author

    philipdumont mannequin commented Jan 22, 2008

    Oh, all right. Just don't tell my boss -- he thinks I'm doing
    "real" work. :-)

    I haven't thoroughly studied the test harness, but it looked like

    <prefix>/bin/python <prefix>/lib/python2.5/test/autotest.py

    was what I should do? The output generated before/after the patch
    was identical.

    @ncoghlan
    Copy link
    Contributor

    The test suite is run via test/regrtest.py. If you aren't on windows,
    'make test' works as well.

    For myself, I tend to just run "./python -m test.regrtest" in the
    directory where I built Python.

    @philipdumont
    Copy link
    Mannequin Author

    philipdumont mannequin commented Feb 12, 2008

    Ok. Ran 'make test' before and after patch. Output identical.
    Attaching output after patch.

    @doerwalter
    Copy link
    Contributor

    The patch no longer applies cleanly to the trunk.

    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Sep 20, 2010

    Any opinions as to whether the patch should be reworked for 3.2 or not?

    @ncoghlan
    Copy link
    Contributor

    I'm rejecting this due to the effect it has on the output parameters.

    The patch ends up have to stick (char *) casts in several places because a pointer into the string provided via the char * input parameter is returned by each affected API to the calling function via a char **output parameter.

    We can't change the signature of the output parameters since that would break existing code. That then means we also can't change the signature of the input parameters, since we can't guarantee that the recipient of the output parameter won't later use it to change things.

    The calling code knows whether or not it is going to modify the pointer it receives back, and hence whether or not it is safe to cast away the const-ness of the passed in pointer.

    @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
    extension-modules C modules in the Modules dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants