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 rollup patch, 2014/01/31 #64655

Closed
larryhastings opened this issue Jan 31, 2014 · 5 comments
Closed

Argument Clinic rollup patch, 2014/01/31 #64655

larryhastings opened this issue Jan 31, 2014 · 5 comments
Assignees
Labels
type-feature A feature request or enhancement

Comments

@larryhastings
Copy link
Contributor

BPO 20456
Nosy @brettcannon, @ncoghlan, @taleinat, @larryhastings, @zware, @serhiy-storchaka, @vajrasky
Files
  • larry.clinic.rollup.2014.01.31.1.diff
  • larry.clinic.rollup.2014.01.31.2.diff
  • fix_clinic_converters_cmd_line.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/larryhastings'
    closed_at = <Date 2014-02-01.06:04:38.433>
    created_at = <Date 2014-01-31.13:18:30.637>
    labels = ['type-feature']
    title = 'Argument Clinic rollup patch, 2014/01/31'
    updated_at = <Date 2014-02-01.15:35:16.226>
    user = 'https://github.com/larryhastings'

    bugs.python.org fields:

    activity = <Date 2014-02-01.15:35:16.226>
    actor = 'vajrasky'
    assignee = 'larry'
    closed = True
    closed_date = <Date 2014-02-01.06:04:38.433>
    closer = 'larry'
    components = []
    creation = <Date 2014-01-31.13:18:30.637>
    creator = 'larry'
    dependencies = []
    files = ['33826', '33841', '33844']
    hgrepos = []
    issue_num = 20456
    keywords = ['patch']
    message_count = 5.0
    messages = ['209784', '209834', '209874', '209875', '209891']
    nosy_count = 8.0
    nosy_names = ['brett.cannon', 'ncoghlan', 'taleinat', 'larry', 'python-dev', 'zach.ware', 'serhiy.storchaka', 'vajrasky']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue20456'
    versions = ['Python 3.4']

    @larryhastings
    Copy link
    Contributor Author

    Probably the last rollup patch for a while; I need to move on to code
    reviewing.

    Here's a list of changes in this patch:

    • I added the oft-requested "rename the C variable for a parameter"
      functionality. That works as follows:

      os.rename
      path as stinky: path_t

      Here the Python name is "path", and the C name is "stinky".
      Internally the "name" of the parameter is the Python name, and
      the "name" of the converter is the C name. (But the converter
      gets the Python name too, because it needs it sometimes, e.g.
      for the keywords array.)

    • Fixed a minor but difficult-to-fix problem: when cloning a function,
      the cloned function would still refer to the original function's
      name. (You can see this in the PATH_T_INITIALIZE macros in
      os_replace in posixmodule.c.)

    • While fixing the above I stumbled over a new problem: rendering
      often changes internal state of a parameter, which could cause
      problems if we clone an already-rendered parameter object. The
      solution: make a copy of all the Parameter objects before rendering,
      then render from the copies.

    • Fixing the above forced me to add a new initalizer to CConverter
      objects: pre_render(), which is called just before rendering.

    • This change also means it's pretty important to call up to the super()
      for converter_init() and pre_render().

    • Fixing the above also required new semantics for converter_init():
      you are no longer permitted to examine the function object in
      that method. (I made it fail noisily with a "LandMine".)

    • "clinic.py --converters" was broken, I fixed it. (It was confused
      by object_converter having format_unit=None.)

    • I fixed the unit tests too.

    • You can now specify blank lines between directives without Clinic
      getting angry at you.

    • I redid how I store all those little C code snippet template.
      Originally they were inline, but flush left, and that made
      reading them tiresome--the indent was jumping around on you.
      Changing to a central list removed the jumping around but now
      you had to scroll back and forth to correlate the template with
      where it was used. The final solution: have them inline again,
      but indented, and simply outdent the text before use. Go ahead,
      tell me that's *not* an improvement. :D (The outdenting function
      uses functools.lru_cache, so processing is really no slower.)

    And last but certainly not least...!

    Following a suggestion by Serhiy Storchaka, Argument Clinic is now
    incredibly sophisticated with respect to #if'd out code. In less than
    200 lines, I wrote a reasonable C preprocessor monitor class. The
    monitor parses C files in parallel to Argument Clinic. At any time
    you can ask the monitor "what's the current preprocessor conditional
    state?" If the current code is potentially if'd out, Argument Clinic
    uses the same conditional to potentially #if out the parser function,
    the docstring, etc, when writing those to buffer or file. It even
    adds this to the end:

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

    But *only* when necessary.

    You can see this in action in the diff for Modules/clinic/zlibmodule.c.h.

    I think this is a huge improvement! (Thanks for the suggestion, Serhiy!)

    @larryhastings larryhastings self-assigned this Jan 31, 2014
    @larryhastings larryhastings added the type-feature A feature request or enhancement label Jan 31, 2014
    @larryhastings
    Copy link
    Contributor Author

    Updated the patch.

    • The "methoddef_ifndef" template is now sent to the "buffer"
      destination by default. I expected posixmodule to have an #ifndef,
      I was surprised to find _import had one too. Both files touched
      to move the buffer to an appropriate spot.

    • Fixed the "original" "preset" so it explicitly sets all three new
      templates just like the actual default.

    • Forgot a minor fix that was in revision 1: when generating the
      docstring for a function using optional groups, suppress the
      "self/type/module" first argument in the signature. (The signature
      isn't parsable by inspect.Signature, so we go ahead and insert
      something user-readable like the docstrings before Argument Clinic
      used to do.)

    • Changed _dbm.dbm.get to no longer use optional groups. Why was it
      doing that in the first place? There's now exactly one function
      checked in using optional groups, curses.window.addch.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 1, 2014

    New changeset 19d81cc213d7 by Larry Hastings in branch 'default':
    #bpo-20456: Several improvements and bugfixes for Argument Clinic,
    http://hg.python.org/cpython/rev/19d81cc213d7

    @larryhastings
    Copy link
    Contributor Author

    Checked in! I think that's the last new feature for Argument Clinic until after 3.4 ships.

    @vajrasky
    Copy link
    Mannequin

    vajrasky mannequin commented Feb 1, 2014

    The converters argument in command line is still broken.

    [sky@localhost cpython3.4]$ hg pull -u
    pulling from http://hg.python.org/cpython
    searching for changes
    no changes found
    [sky@localhost cpython3.4]$ ./python Tools/clinic/clinic.py --converters

    Legacy converters:
    Traceback (most recent call last):
      File "Tools/clinic/clinic.py", line 4131, in <module>
        sys.exit(main(sys.argv[1:]))
      File "Tools/clinic/clinic.py", line 4063, in main
        print('    ' + ' '.join(c for c in legacy if c[0].isupper()))
      File "Tools/clinic/clinic.py", line 4063, in <genexpr>
        print('    ' + ' '.join(c for c in legacy if c[0].isupper()))
    IndexError: string index out of range

    Unit test for exercising Tools/clinic/clinic.py using assert_python_ok would be good, but that deserves a dedicated ticket.

    Here is the patch to fix the bug.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    AA-Turner pushed a commit to AA-Turner/devguide that referenced this issue Sep 13, 2023
    erlend-aasland pushed a commit to python/devguide that referenced this issue Sep 26, 2023
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant