Skip to content

Conversation

@KDr2
Copy link
Member

@KDr2 KDr2 commented Jun 18, 2021

No description provided.

Copy link
Member

@yebai yebai left a comment

Choose a reason for hiding this comment

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

thanks, @KDr2 - looks good! I left some minor comments below.

Also, I would suggest that we keep the memlayout.cpp file under Libtask_jll. Assuming that we need to re-run memlayout inside a Github workflow, in order to get the member layout for a new Julia release, we can import Libtask_jll. However, this step can be done after the code becomes more mature to avoid frequent changes to Libtask_jll during the development phase.

@KDr2 KDr2 force-pushed the memlayout branch 6 times, most recently from 0bdbb43 to 2f200b2 Compare June 24, 2021 04:41
@KDr2 KDr2 marked this pull request as ready for review June 24, 2021 05:17
@KDr2 KDr2 requested a review from yebai June 24, 2021 05:17
@KDr2
Copy link
Member Author

KDr2 commented Jun 24, 2021

Now, it's ready to be reviewed and merged.

Copy link
Member

@yebai yebai left a comment

Choose a reason for hiding this comment

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

Looks good to me overall - one minor comment: maybe create a subfolder in workflows for tasklayout and store the Julia script and cpp source file inside tasklayout?

@devmotion This PR is attempting to crate a more maintainable version of Libtask by doing most of the task struct manipulation operations on the Julia side, and minimise the operations on the C side (i.e. in Libtask_jll). The solution here is not yet perfect, but operational, and already more generic than the current mechanism (e.g. it works correctly on Julia v.1.5.4 and v1.5.3). Can you take a look and see whether you have any comments?

@KDr2
Copy link
Member Author

KDr2 commented Jun 25, 2021

maybe create a subfolder in workflows for tasklayout and store the Julia script and cpp source file inside tasklayout?

Done.

@yebai yebai merged commit 7c404c4 into master Jun 26, 2021
@delete-merged-branch delete-merged-branch bot deleted the memlayout branch June 26, 2021 12:20
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