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

gh-112068: C API: Add support of nullable arguments in PyArg_Parse (suffix) #121303

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Jul 3, 2024

This is a variant of #121187, but with suffix instead of prefix. i? instead of ?i.

Copy link
Contributor

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

I'm really sorry that you needed to implement the suffix just for me to actually understand that it's probably harder to maintain and to extend compared to the prefix case.

The only reason why I would advocate for the suffix is more because of its use in other languages but maybe I shouldn't have kept that argument at the top of my list and should have rather go for a more simple implementation (for instance in TypeScript, you write function sumTwoMaybeThree(x: number, y: number, z?: number): number and in other languages as I said you have the ?. operator.

In addition, since (...)? is not yet implemented, I assume that it would be even more complex since you need to open / close contexts and after all that, you still need to parse that ?. However ?(...) would be easier to implement I think since you could add the flag beforehand and process (...) normally.

By the way, is there a reason why we have a huge switch instead of some separate functions like in _testcapi/getargs.c? is it only for efficiency purposes and to reduce function calls (though those could be probably inlined)?

@@ -616,6 +624,15 @@ convertsimple(PyObject *arg, const char **p_format, va_list *p_va, int flags,
char *msgbuf, size_t bufsize, freelist_t *freelist)
{
#define RETURN_ERR_OCCURRED return msgbuf
#define HANDLE_NULLABLE \
Copy link
Contributor

Choose a reason for hiding this comment

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

I know understand why the implementation is more complex. You essentially need to add that macro after every flag that is parsed instead of handling it as if it were a regular character.

@serhiy-storchaka
Copy link
Member Author

You essentially need to add that macro after every flag that is parsed instead of handling it as if it were a regular character.

In case of multicharacter format unit you need to reorganize the code and add new code at all right places. This is actually simpler than I thought, because the same macros can be used in all cases (except (...)?), but this is still more complex than the prefix implementation, and more error-prone.

For (...)?, when it encounters an opening (, it needs to search the closing ) and look if there is a following ?. Note that parentheses can be nested, so this is not so trivial. If the closing )? is found and the argument is None, it should scan the format string again and skip PyArg_Parse() arguments corresponded to nested format units. The cost of additional scanning is played every time, even if the new feature is not used. Well, this can be optimized for common cases, but at the cost of more complicated code.

@serhiy-storchaka serhiy-storchaka deleted the zipimport-namespace-suffix branch July 4, 2024 08:05
@serhiy-storchaka serhiy-storchaka restored the zipimport-namespace-suffix branch July 4, 2024 08:06
@serhiy-storchaka
Copy link
Member Author

I managed to implement (...)? without affecting performance. But the code is still more complex. I need to check it many more times.

iritkatriel added a commit to iritkatriel/cpython that referenced this pull request Aug 23, 2024
iritkatriel added a commit to iritkatriel/cpython that referenced this pull request Aug 23, 2024
@serhiy-storchaka serhiy-storchaka marked this pull request as ready for review November 8, 2024 16:33
@serhiy-storchaka serhiy-storchaka marked this pull request as draft November 8, 2024 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants