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

Argument Clinic: multiple macro definition #67688

Closed
serhiy-storchaka opened this issue Feb 22, 2015 · 27 comments
Closed

Argument Clinic: multiple macro definition #67688

serhiy-storchaka opened this issue Feb 22, 2015 · 27 comments
Assignees
Labels
build The build process and cross-build topic-argument-clinic type-bug An unexpected behavior, bug, or error

Comments

@serhiy-storchaka
Copy link
Member

BPO 23500
Nosy @larryhastings, @serhiy-storchaka
Files
  • sample.c
  • clinic_append.patch
  • sample.c: Fixed sample file
  • larry.ac_multiple_macro_definitions.diff.1.txt
  • larry.ac_multiple_macro_definitions.diff.2.txt: Diff after running "make clinic" with this patch
  • clinic_append_2.patch
  • sample.c
  • larry.ac_multiple_macro_definitions.diff.3.txt: Another approach for cleaning up the multiple macros problem.
  • larry.ac_multiple_macro_definitions.diff.4.txt
  • larry.ac_multiple_macro_definitions.diff.5.txt
  • 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/larryhastings'
    closed_at = <Date 2015-04-03.20:35:59.511>
    created_at = <Date 2015-02-22.10:37:42.845>
    labels = ['type-bug', 'expert-argument-clinic', 'build']
    title = 'Argument Clinic: multiple macro definition'
    updated_at = <Date 2015-04-03.20:35:59.509>
    user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2015-04-03.20:35:59.509>
    actor = 'larry'
    assignee = 'larry'
    closed = True
    closed_date = <Date 2015-04-03.20:35:59.511>
    closer = 'larry'
    components = ['Build', 'Demos and Tools', 'Argument Clinic']
    creation = <Date 2015-02-22.10:37:42.845>
    creator = 'serhiy.storchaka'
    dependencies = []
    files = ['38205', '38328', '38329', '38491', '38492', '38494', '38495', '38538', '38821', '38822']
    hgrepos = []
    issue_num = 23500
    keywords = ['patch', 'needs review']
    message_count = 27.0
    messages = ['236403', '236405', '236407', '237183', '237185', '237192', '237201', '238076', '238119', '238120', '238122', '238126', '238128', '238384', '238420', '239576', '239596', '240008', '240009', '240010', '240012', '240016', '240028', '240029', '240030', '240031', '240035']
    nosy_count = 3.0
    nosy_names = ['larry', 'python-dev', 'serhiy.storchaka']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue23500'
    versions = ['Python 3.5']

    @serhiy-storchaka
    Copy link
    Member Author

    Argument Clinic generates multiple definitions in non-block mode for functions with alternating signature. See example file. If FOO is not defines, SPAM_METHODDEF is defined twice.

    First time in:

    #ifndef SPAM_METHODDEF
        #define SPAM_METHODDEF
    #endif /* !defined(SPAM_METHODDEF) */

    Second time in:

    #if !defined(FOO)
    ...
    #define SPAM_METHODDEF    \

    @serhiy-storchaka serhiy-storchaka added build The build process and cross-build type-bug An unexpected behavior, bug, or error labels Feb 22, 2015
    @larryhastings
    Copy link
    Contributor

    Can you give me a test case?

    @serhiy-storchaka
    Copy link
    Member Author

    Oh, sorry. Here is a sample. Other sample see in bpo-23501.

    @serhiy-storchaka
    Copy link
    Member Author

    Here is a patch which moves all methoddef_ifndef to the end of the buffer or file, so they are not conflicts with other definitions.

    @serhiy-storchaka
    Copy link
    Member Author

    Here is a sample file generated by fixed clinic.py.

    @larryhastings
    Copy link
    Contributor

    In your sample output we still get two #ifndef SPAM_METHODDEF stanzas. Wouldn't it be better to only have one?

    Maybe Clinic needs to be smarter about generating those anyway. Let me think about it.

    @serhiy-storchaka
    Copy link
    Member Author

    Actually in this sample output no one #ifndef SPAM_METHODDEF stanza is needed, because SPAM_METHODDEF is defined in any case.

    @serhiy-storchaka
    Copy link
    Member Author

    May be first commit this non-perfect solution? Generated code is correct, it is just not optimal. This issue is a dependency of bpo-23501, that is a dependency of bpo-23492, that is a dependency of my patch for optimizing argument parsing in 1-argument functions.

    @larryhastings
    Copy link
    Contributor

    How about this approach? Only ever emit the #ifndef stanza once per symbol.

    @larryhastings
    Copy link
    Contributor

    (see larry.ac_multiple_macro_definitions.diff.1.txt posted above)

    @larryhastings
    Copy link
    Contributor

    Oops, I should have run "make clinic", so you could see all the changes that result from this patch.

    @serhiy-storchaka
    Copy link
    Member Author

    It doesn't fix the issue, because the #ifndef stanza is emitted before second definition. Try to run clinic.py with your patch on sample.c. But may be this idea can be used with my patch.

    @serhiy-storchaka
    Copy link
    Member Author

    Yes, this works. Here is combined patch and proceeded sample file.

    @larryhastings
    Copy link
    Contributor

    What do you think of this approach? Now a "Destination" object behaves like an array of text accumulators. If you ask for one that doesn't exist it's created for you. When the Destination is dumped, the output from each accumulator is joined together, like buffer[0] + buffer[1] + buffer[2]. (You can even specify negative indices, if you want text that goes *before* the default text accumulator.)

    With this approach, all the #ifndef stanzas are at the end of the emitted text.

    @serhiy-storchaka
    Copy link
    Member Author

    This looks as generalization of my patch. It produces the same output. I left comments on Rietveld, but in any case the patch LGTM.

    @serhiy-storchaka
    Copy link
    Member Author

    Could you please commit your patch (may be with changes) Larry?

    @larryhastings
    Copy link
    Contributor

    I want to redo it--it's smelly. I hope to get it done this week.

    @larryhastings
    Copy link
    Contributor

    As promised, here's a cleaned-up version of the patch. The taxonomy of objects now makes sense: a Destination contains a BufferSeries object, rather than Destinations weirdly supporting __getitem__ to reference different objects.

    I tripped over myself a little with the "two-pass" destination / preset. It was an experiment long ago but nobody's using it. So rather than fix it I just turned it off.

    @serhiy-storchaka
    Copy link
    Member Author

    Looks as you didn't notice my comments on Rietveld.

    @larryhastings
    Copy link
    Contributor

    I did, I just didn't respond. I'll do that now.

    @larryhastings
    Copy link
    Contributor

    Updated patch, removed all references to two-pass. Also realized I needed to make the default behavior for methoddef_ifndef go to the end. And, finally, I forgot to merge the "only print each #ifndef block once" code I wrote before when I redid the patch, so that's in now.

    @serhiy-storchaka
    Copy link
    Member Author

    larry.ac_multiple_macro_definitions.diff.5.txt LGTM.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 3, 2015

    New changeset 25eef0ecb9c1 by Larry Hastings in branch 'default':
    Issue bpo-23500: Argument Clinic is now smarter about generating the "#ifndef"
    https://hg.python.org/cpython/rev/25eef0ecb9c1

    @larryhastings
    Copy link
    Contributor

    Does this really need a backport to 3.4?

    @serhiy-storchaka
    Copy link
    Member Author

    I think this is not needed.

    @serhiy-storchaka
    Copy link
    Member Author

    Thank you Larry.

    @larryhastings
    Copy link
    Contributor

    Removing 3.4 from the version list as I close the bug, then. If we decide we need it backported please reopen (or create a new bug, either is fine).

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    erlend-aasland added a commit to erlend-aasland/cpython that referenced this issue May 3, 2023
    The code that manipulated 'second_pass_replacements' was removed in 2015
    with commit 0759f84 (pythongh-67688, bpo-23500).
    erlend-aasland added a commit to erlend-aasland/cpython that referenced this issue May 3, 2023
    The code that manipulated 'second_pass_replacements' was removed in 2015
    with commit 0759f84 (pythongh-67688, bpo-23500).
    erlend-aasland added a commit that referenced this issue May 4, 2023
    …104147)
    
    The code that manipulated 'second_pass_replacements' was removed in 2015
    with commit 0759f84 (gh-67688, bpo-23500).
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    build The build process and cross-build topic-argument-clinic type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants