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/357/use alignment from memory helper #358

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

aaroniz-bgu
Copy link
Contributor

Closes #357.

@kkard2
Copy link
Contributor

kkard2 commented Nov 7, 2023

was there a reason for deleting the temp var? probably doesn't matter tho

@aaroniz-bgu
Copy link
Contributor Author

was there a reason for deleting the temp var? probably doesn't matter tho

I could add that back, thought it should be deleted by the issue's description. Might be worth to add it back in-case these calls are expensive.
var size = MemoryHelper.AlignmentOf<T>();

@kkard2
Copy link
Contributor

kkard2 commented Nov 7, 2023

oh you're right, issue sounds like that, but it probably wasn't intended that way.
i doubt these calls are expensive (altho everything is relative) but i would argue it's more readable with temp var

@kkard2
Copy link
Contributor

kkard2 commented Nov 7, 2023

remember to use curly braces on the previous line (link).

also only now i noticed, you should pull master and branch out from here, currently the history might get goofed up (there are commits from previous prs)

@aaroniz-bgu
Copy link
Contributor Author

IDE does that by default need to disable that. Yep noticed that too, for some reason the sync didn't work as expected

@kkard2
Copy link
Contributor

kkard2 commented Nov 7, 2023

i would probably still prefer doing it on a new branch based on master and not including this mess in the repo (if we squashed everything this wouldn't be an issue...)

otherwise lgtm

@aaroniz-bgu aaroniz-bgu force-pushed the fix/357/use-alignment-from-memory-helper branch from a58ba54 to 6dbe43e Compare November 7, 2023 15:57
@aaroniz-bgu
Copy link
Contributor Author

branch history fixed

@kkard2
Copy link
Contributor

kkard2 commented Nov 8, 2023

vi changed MemoryHelper to InteropMemoryHelper (and its namespace) on master (i think it was after your fix so not your fault); this will probably fail checks after merge

you should rebase/merge master into this branch and fix compilation errors

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.

Use alignment from InteropMemoryHelper
2 participants