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

Passing plain identifier inside HPX_DEFINE_PLAIN_ACTION_1 #1550

Closed
AntonBikineev opened this issue May 25, 2015 · 8 comments
Closed

Passing plain identifier inside HPX_DEFINE_PLAIN_ACTION_1 #1550

AntonBikineev opened this issue May 25, 2015 · 8 comments

Comments

@AntonBikineev
Copy link
Contributor

HPX_DEFINE_PLAIN_ACTION_1 and HPX_DEFINE_PLAIN_DIRECT_ACTION_1 pass plain "name" as an identifier for action name, which may cause class redefinition.

It might be reasonable to pass BOOST_PP_CAT(func, _action) like it's done for HPX_PLAIN_ACTION or just to remove these macros.

@K-ballo
Copy link
Member

K-ballo commented May 25, 2015

Introduced by eb93bb0

@hkaiser
Copy link
Member

hkaiser commented May 25, 2015

This was fixed by 2bdf6a5

@K-ballo
Copy link
Member

K-ballo commented May 25, 2015

That commit looks like a functional change. Does it require documentation changes to follow? What about announcing the breakage on the mailing list? Where are the basic unit tests to catch this kind of bug?

This kind of change should have gone through a pull request.

@AntonBikineev
Copy link
Contributor Author

@K-ballo I don't think it's functional, because it doesn't break usage of macro and any existing code. Documentation defines it exactly as it is now. Sorry for committing directly to master.

@K-ballo
Copy link
Member

K-ballo commented May 25, 2015

Whether the change breaks out-of-tree builds depends only on what's documented. The current documentation for HPX_DEFINE_PLAIN_ACTION depicts it as a variadic macro. It looks suspiciously underdocumented. Do we want it to diverge from HPX_PLAIN_ACTION and HPX_REGISTER_ACTION? Do we want it to diverge from HPX_DEFINE_COMPONENT_ACTION?

@hkaiser
Copy link
Member

hkaiser commented May 25, 2015

@K-ballo you're probably right, we should leave it as an variadic macro and try to implement the one argument version analogously to the existing macros. Let's move this to a PR as well.

@hkaiser
Copy link
Member

hkaiser commented May 26, 2015

A proper fix has been added to the branch fixing_1550.

@hkaiser
Copy link
Member

hkaiser commented May 26, 2015

Fixed by merging #1553

@hkaiser hkaiser closed this as completed May 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants