GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-37540: vectorcall: keyword names must be strings by jdemeyer · Pull Request #14682 · python/cpython

Conversation

@jdemeyer

@jdemeyer jdemeyer commented Jul 10, 2019 •
edited by miss-islington
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

The fact that keyword names are strings is now part of the vectorcall and METH_FASTCALL protocols. The biggest concrete change is that _PyStack_UnpackDict now checks that and raises TypeError if not.

CC @markshannon @vstinner

https://bugs.python.org/issue37540

Automerge-Triggered-By: @encukou

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@jdemeyer
bpo-37540: vectorcall: keyword names must be strings
786d45f
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Jul 10, 2019
@bedevere-bot bedevere-bot added the awaiting review label Jul 10, 2019
jdemeyer added 2 commits July 10, 2019 12:30
@jdemeyer
bpo-37540: update doc and NEWS
b66e840
@jdemeyer
bpo-37540: fixup
723334c
@encukou

encukou commented Jul 13, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Instead of "string", I'd say "`str` or a subclass" explicitly.
(In documentation; not necessarily in error messages)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jdemeyer

jdemeyer commented Jul 15, 2019

Copy link
Copy Markdown
Contributor Author

There are many instances in the documentation (unrelated to vectorcall) where the word "string" is used. I don't see the problem here.

Concrete proposal: we leave this doc as is and I will reboot #13844 (I always planned to do that, but after all other PRs that affect documentation). In the section describing the "vectorcall protocol", I will say explicitly what "string" means for vectorcall.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jdemeyer

jdemeyer commented Jul 19, 2019

Copy link
Copy Markdown
Contributor Author

Is this PR okay now or do you want me to change things?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@encukou

encukou commented Jul 19, 2019

Copy link
Copy Markdown
Member

@markshannon, you said you were looking at this PR. Is that still the case? Any progress?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jdemeyer

jdemeyer commented Aug 4, 2019

Copy link
Copy Markdown
Contributor Author

Ping?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jdemeyer jdemeyer mentioned this pull request Aug 5, 2019
bpo-37207: Use PEP 590 vectorcall to speed up range(), list() and dict() by about 30% #13930
Closed
encukou
encukou approved these changes Aug 15, 2019
View reviewed changes

@encukou encukou left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@markshannon seems unresponsive. I don't see an issue with this PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_unpack_ex.py
>>> f(**{1: 3}, **{1: 5})
Traceback (most recent call last):
...
TypeError: f() keywords must be strings

@encukou encukou Aug 15, 2019

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see f(**{1:2}) is still tested in test_excall.py.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jdemeyer jdemeyer Aug 15, 2019

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. This is testing two errors at the same time (duplicate keyword and non-string keyword). Which of the two errors you get seems arbitrary to me and this PR changes the error.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Aug 15, 2019
@encukou encukou added the 🤖 automerge label Aug 16, 2019
@miss-islington
miss-islington merged commit 0567786 into python:master Aug 16, 2019
@bedevere-bot bedevere-bot removed the awaiting merge label Aug 16, 2019
@jdemeyer
jdemeyer deleted the vectorcall_strings branch August 16, 2019 11:23
@jdemeyer

jdemeyer commented Aug 16, 2019

Copy link
Copy Markdown
Contributor Author

Thanks! This will help to make progress on some other PRs.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jdemeyer jdemeyer mentioned this pull request Aug 16, 2019
bpo-29312: use METH_FASTCALL for dict.update #14589
Closed
lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019
@jdemeyer @lisroach
bpo-37540: vectorcall: keyword names must be strings (pythonGH-14682)
ac678b6 
The fact that keyword names are strings is now part of the vectorcall and `METH_FASTCALL` protocols. The biggest concrete change is that `_PyStack_UnpackDict` now checks that and raises `TypeError` if not.

CC @markshannon @vstinner 


https://bugs.python.org/issue37540
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
@jdemeyer @DinoV
bpo-37540: vectorcall: keyword names must be strings (pythonGH-14682)
1cced66 
The fact that keyword names are strings is now part of the vectorcall and `METH_FASTCALL` protocols. The biggest concrete change is that `_PyStack_UnpackDict` now checks that and raises `TypeError` if not.

CC @markshannon @vstinner 


https://bugs.python.org/issue37540
websurfer5 pushed a commit to websurfer5/cpython that referenced this pull request Jul 20, 2020
@jdemeyer @websurfer5
bpo-37540: vectorcall: keyword names must be strings (pythonGH-14682)
20724fe 
The fact that keyword names are strings is now part of the vectorcall and `METH_FASTCALL` protocols. The biggest concrete change is that `_PyStack_UnpackDict` now checks that and raises `TypeError` if not.

CC @markshannon @vstinner 


https://bugs.python.org/issue37540
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@encukou encukou encukou approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@jdemeyer @encukou @the-knights-who-say-ni @bedevere-bot @miss-islington
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.