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

fix/program-size4: fix padding in edge cases #637

Closed
wants to merge 2 commits into from
Closed

Conversation

naure
Copy link
Collaborator

@naure naure commented Nov 26, 2024

This bug was fixed then broken again in a cleanup attempt. 🤦

Note that the whole topic is being refactored in #624 so hopefully we won’t have this issue anymore.

@naure naure requested review from hero78119 and mcalancea November 26, 2024 10:25
@naure naure enabled auto-merge (squash) November 26, 2024 10:31
@naure naure requested a review from lispc November 26, 2024 10:32
@hero78119 hero78119 disabled auto-merge November 26, 2024 10:45
Copy link
Collaborator

@hero78119 hero78119 left a comment

Choose a reason for hiding this comment

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

Sorry I disable auto merge because I was thinking to identify the root cause instead of quick patching 😃 .

question, does the number of instance size different with real padding just exist to deal with program table special case?

@naure
Copy link
Collaborator Author

naure commented Nov 26, 2024

Yes at the moment this problem only exists the program table because it has a separate configuration of max size versus content. In other places (e.g. NonVolatileTableConfig) we just required an exact match.

Although you’re right that there is a more elegant fix which is to require a tight fit of the size. This wasn’t possible in the original fix because the size was not configurable yet. Let me try that.

@matthiasgoergens
Copy link
Collaborator

Thanks for trying to fix this.

Could we please add a test that would fail if/when this break again?

@naure
Copy link
Collaborator Author

naure commented Nov 26, 2024

Better not introduce this problem in the first place: #638

@naure naure closed this Nov 26, 2024
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.

4 participants