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

Issue8 #16

Closed
wants to merge 2 commits into from
Closed

Issue8 #16

wants to merge 2 commits into from

Conversation

kamuik16
Copy link
Contributor

Description

Draft PR for #8

Additional context

I have added some initial logic to support __gap storage gaps variable.

Copy link

sonarcloud bot commented Jan 10, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@kamuik16 kamuik16 mentioned this pull request Jan 10, 2024
@ZeroEkkusu ZeroEkkusu linked an issue Jan 10, 2024 that may be closed by this pull request
Copy link
Member

@ZeroEkkusu ZeroEkkusu left a comment

Choose a reason for hiding this comment

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

Okay, I've done some research and we're going to set one rule:

__gaps must be of size 32 * n. You don't need to worry about it because it's already what we're doing.
Just a note to self that we may want to throw an error if that isn't the case.


  1. We'll need problematicOnlyBecauseOfGap function:

IN: item from new layout, old layout
OUT: whether item from new layout overlaps only with gap from old layout

The function just needs to iterate over the items from the old layout, and using the current item's start and end, check if there's an overlap with the provided item from the new layout (by comparing to its start and end).
If there's an overlap, return false if the current item is not a __gap.
If it is a __gap, just continue.
Return true at the end, i.e., if we don't find an overlap with an item from the old layout that is not a __gap.

This should be straightforward to do.


Let's start from there - by adding this function. I'll continue to debug the code in the meantime.
Let me know if you think this is a good idea.

@kamuik16
Copy link
Contributor Author

Okay, I've done some research and we're going to set one rule:

__gaps must be of size 32 * n. You don't need to worry about it because it's already what we're doing. Just a note to self that we may want to throw an error if that isn't the case.

  1. We'll need problematicOnlyBecauseOfGap function:

IN: item from new layout, old layout OUT: whether item from new layout overlaps only with gap from old layout

The function just needs to iterate over the items from the old layout, and using the current item's start and end, check if there's an overlap with the provided item from the new layout (by comparing to its start and end). If there's an overlap, return false if the current item is not a __gap. If it is a __gap, just continue. Return true at the end, i.e., if we don't find an overlap with an item from the old layout that is not a __gap.

This should be straightforward to do.

Let's start from there - by adding this function. I'll continue to debug the code in the meantime. Let me know if you think this is a good idea.

This is the same as variableFitInGap function, isn't it?

@ZeroEkkusu
Copy link
Member

ZeroEkkusu commented Jan 10, 2024

It's another, separate function.

variableFitInGap check's if the variable fits in the gap wholly. That is to say, it starts and ends in the gap.

problematicOnlyBecauseOfGap checks for a partial overlap, and we'll use it to cover an edge case later.

Here's an example:

var starts at position 0.
gap starts at position 1.

0   var
1   gap

Let's say that we change the type of var to a type that occupies more space.

var starts at position 0, but is of a bigger type and occupies position 1 as well.
gap now starts at position 2.

0   biggerVar
2   gap

If you think about it, var now occupies both the space that it previously occupied (0) AND some space that the gap used to occupy (1).

We'll need a function to check for that, and that's why we're adding it.

Let me know if that makes sense.

@kamuik16
Copy link
Contributor Author

It's another, separate function.

variableFitInGap check's if the variable fits in the gap wholly. That is to say, it starts and ends in the gap.

problematicOnlyBecauseOfGap checks for a partial overlap, and we'll use it to cover an edge case later.

I think it's probably good if you take it on from here, because you will have to explain each and everything and it will consume your much of time so maybe I'll close this PR and let you do your job. What do you think?

@ZeroEkkusu
Copy link
Member

Whatever you prefer. I have something else to work on, so I won't get back to Storage Delta this week. Totally fine if you want or not want to give it a shot in the meantime.

@kamuik16
Copy link
Contributor Author

Whatever you prefer. I have something else to work on, so I won't get back to Storage Delta this week. Totally fine if you want or not want to give it a shot in the meantime.

Okay @ZeroEkkusu! Btw it was great doing discussion with you on this and I definitely learned a lot! Thanks! Closing this PR.

@kamuik16 kamuik16 closed this Jan 10, 2024
@ZeroEkkusu
Copy link
Member

Thank you,
Take care, @kamuik16!

@kamuik16 kamuik16 deleted the issue8 branch January 10, 2024 14:26
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.

Add support for __gap
2 participants