GeezerVison

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@serhiy-storchaka I have implemented the same logic for module and class annotations, so all annotations will be computed at compilation time.

I have made the requested changes; please review again

Thanks for making the requested changes!

@serhiy-storchaka: please review the changes made to this pull request.

Rather than return the tuple size via a pointer (out) parameter, you can return the size directly as it cannot be negative, leaving -1 to signify an error. So,

int compiler_visit_argannotation(struct compiler *c, identifier id, expr_ty annotation, Py_ssize_t *annotations_len);

becomes

int compiler_visit_argannotation(struct compiler *c, identifier id, expr_ty annotation);

and the use changes from

if (compiler_visit_argannotation(..., &len))
    goto error;

to

len = compiler_visit_argannotation(...);
if (len < 0)
    goto error;

Rather than return the tuple size via a pointer (out) parameter, you can return the size directly as it cannot be negative,
leaving -1 to signify an error.

I think it will make code more complicated because it will require rewriting all ifs for compiler_visit_annotations function.

It will be great if compiler_visit_argannotations and compiler_visit_argannotation will follow the same conventions - return tuple size through the pointer or as return value.

@markshannon What is your opinion regarding that?

I prefer my approach (I wouldn't have suggested it otherwise 🙂 ), but the code is fine as it is.

I'm still happy with merging it.

I totally agree with you, personally, I don't like when the return value passed through the pointer instead of simple return, as zen said explicit is better than implicit.

But otherwise, In my opinion, it will make code less readable.
From this:

if (!compiler_visit_argannotations(c, args->args, &annotations_len))
   return 0;
if (args->vararg && args->vararg->annotation &&
    !compiler_visit_argannotation(c, args->vararg->arg,
                                  args->vararg->annotation, &annotations_len))
   return 0;

It will be rewritten to:

    len = compiler_visit_argannotations(c, args->args);
    if (len < 0)
        return 0;
    annotations_len += len;
    if (args->vararg && args->vararg->annotation) {
        len = compiler_visit_argannotation(c, args->vararg->arg,
                                           args->vararg->annotation);
        if (len < 0)
            return 0;
        annotations_len += len;
    }

This situation is a double-edged sword when we should choose between code readability and code explicity.

@Fidget-Spinner Thank you for making docs more clear and for adding Inada to the list of contributors. It's my bad, I must definitely add Inada to the contributor's list because this PR is fully his idea.

Can someone rerun Azure Pipelines?
I believe it failed not because of my code)

Even core committers can not re-run Azure Pipeline. Close and reopen is the most easy way to rebuild, although it starts not only Azure Pipeline.

@methane Looks like everything is done and this PR can be merged

I did make regen-importlib but I could not git push to your repository. Would you do it?

I did make regen-importlib but I could not git push to your repository. Would you do it?

Sure, no problems.

@methane I have regenerated import-lib

Congrats, @uriyyo. And thank you reviewers.

Thanks all, I am happy that we merge this PR💫

@methane Should we do smth similar for class/module annotations?