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

Move Artifact Register FrameInfo #4011

Merged
merged 20 commits into from
Jun 29, 2023
Merged

Move Artifact Register FrameInfo #4011

merged 20 commits into from
Jun 29, 2023

Conversation

ptitSeb
Copy link
Contributor

@ptitSeb ptitSeb commented Jun 20, 2023

Move Artifact Register FrameInfo to a more fitting place. Also register_frameinfo needs a &mut self now to reflect it change a state (even if it's a global static)

@ptitSeb ptitSeb requested a review from syrusakbary as a code owner June 20, 2023 09:53
Copy link
Contributor

@theduke theduke left a comment

Choose a reason for hiding this comment

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

I don't feel comfortable approving here, because I don't know the code and context.

Is @syrusakbary familiar with this?

Otherwise will dig in a bit more to understand it properly.

lib/compiler/src/engine/artifact.rs Outdated Show resolved Hide resolved
lib/compiler/src/engine/artifact.rs Show resolved Hide resolved
Copy link
Member

@syrusakbary syrusakbary left a comment

Choose a reason for hiding this comment

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

Would it be possible to have a test verifying that the collision is fixed?

@ptitSeb
Copy link
Contributor Author

ptitSeb commented Jun 27, 2023

Would it be possible to have a test verifying that the collision is fixed?

Not sure how. The minimal test written by cosmos doesn't crash all the time. And it's a mater of timing here.

@ptitSeb ptitSeb requested a review from syrusakbary June 27, 2023 13:05
@webmaster128
Copy link
Contributor

I started to run this patch in infinite loop in our full test suite. There the issue popped up consistently before. Looks good for now. Will let you know if I find something.

@webmaster128
Copy link
Contributor

With this change I cannot reproduce the issue #3793 anymore. I ran the project's tests for more than 1000 times now. Looking good 🎉

@ptitSeb ptitSeb added this to the v4.1 milestone Jun 28, 2023
@ptitSeb ptitSeb added the priority-medium Medium priority issue label Jun 28, 2023
@ptitSeb ptitSeb merged commit c7b0e7f into master Jun 29, 2023
@ptitSeb ptitSeb deleted the move_register_frameinfo branch June 29, 2023 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority-medium Medium priority issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants