Skip to content

bpo-41746: Fix CHECK macros to not do type erasure #22786

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

Closed
wants to merge 1 commit into from

Conversation

lysnikolaou
Copy link
Member

@lysnikolaou lysnikolaou commented Oct 19, 2020

@lysnikolaou
Copy link
Member Author

@gvanrossum @pablogsal One question on this:

In the macro #define CHECK(result) (result == NULL ? SET_ERROR(p) : result) result might be a call to some costly function, which in the positive case will be called twice. Are we confident enough the the compiler is going to optimize those doubled calls away?

Copy link
Member

@pablogsal pablogsal left a comment

Choose a reason for hiding this comment

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

Unfortunately, this does not work because the ternary operator deduces the type as void*. From the C99 standard:

since C does not have a type hierarchy for pointer types, pointer operands may only be used if they are of the same type (ignoring type qualifiers) or one is void or NULL.

@bedevere-bot
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@pablogsal
Copy link
Member

The grammar needs to be fixed as well. If you substitute the macro for an identity:

# define CHECK(result) result

you will start to see these errors:

n file included from Parser/pegen.h:7,
                 from Parser/parser.c:2:
./Include/Python-ast.h:647:50: note: expected ‘asdl_expr_seq *’ but argument is of type ‘asdl_seq *’
  647 | expr_ty _Py_BoolOp(boolop_ty op, asdl_expr_seq * values, int lineno, int
      |                                  ~~~~~~~~~~~~~~~~^~~~~~
In file included from Parser/parser.c:2:

The reason these errors are not present now is that the macros pass void* and that can be cast to anything, but that's what we try to avoid.

@lysnikolaou
Copy link
Member Author

I'm closing this.

@gvanrossum
Copy link
Member

Why?

@lysnikolaou
Copy link
Member Author

In a chat I had with @pablogsal, he explained to me why this doesn't work. Basically the different pointer types will get cast to void *s with this solution as well. The easiest to grasp solution (Pablo's idea) would be to also pass the return type to the macro and cast to that type. That would require a good amount of changes to the grammar though.

Bottom line is we should probably discuss this a bit more. Another idea Pablo mentioned is to pass the type to the macro in the generator level, but that hardly seems feasible for nested call to CHECK.

@gvanrossum
Copy link
Member

Hm, it's just a bit of work, we should just do it.

@lysnikolaou
Copy link
Member Author

Opened #22864.

@lysnikolaou lysnikolaou deleted the bpo-41746 branch October 21, 2020 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants