Skip to content

Conversation

@steven-johnson
Copy link
Contributor

Moves the ad-hoc implementation our of HostClosure::arguments() for easier debugging usage. Also, drive-by elimination of the body of HostClosure ctor, which was identical to the one inherited from Closure.

Moves the ad-hoc implementation our of HostClosure::arguments() for easier debugging usage. Also, drive-by elimination of the body of HostClosure ctor, which was identical to the one inherited from Closure.
@steven-johnson
Copy link
Contributor Author

Something about this has actually broken OpenCL code, not sure why -- maybe the HostClosure change, somehow?

@steven-johnson
Copy link
Contributor Author

This "works" but should not be landed until we understand how/why the HostClosure change broke something.

@steven-johnson steven-johnson marked this pull request as draft November 26, 2021 23:34
@steven-johnson steven-johnson marked this pull request as ready for review December 2, 2021 01:15
@steven-johnson
Copy link
Contributor Author

The failure turned out to be don't-call-virtual-methods-from-a-ctor. Added an explanatory comment so that people won't make the same mistake I did.

@steven-johnson steven-johnson merged commit 0ed461b into master Dec 2, 2021
@steven-johnson steven-johnson deleted the srj/closure-dump branch December 2, 2021 18:38
steven-johnson added a commit that referenced this pull request Dec 2, 2021
The current API requires calling a Visitor from the Closure ctor, which means we implicitly call virtual methods from the class ctor, which is a no-no for a non-final class (see comments on #6443).
@steven-johnson steven-johnson mentioned this pull request Dec 2, 2021
steven-johnson added a commit that referenced this pull request Dec 2, 2021
The current API requires calling a Visitor from the Closure ctor, which means we implicitly call virtual methods from the class ctor, which is a no-no for a non-final class (see comments on #6443).
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.

3 participants