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

update system methods. #1155

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

Conversation

Jim8y
Copy link
Contributor

@Jim8y Jim8y commented Aug 25, 2024

No description provided.

Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

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

You also moved some methods and it complicate the review :(

@@ -59,6 +59,12 @@ static Instruction()
}
}

internal Instruction AddTarget(JumpTarget target)
{
target.Instruction = this;
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't add the NOP, isn't it?

Copy link
Contributor Author

@Jim8y Jim8y Aug 25, 2024

Choose a reason for hiding this comment

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

This one will not, we can set jump target to existing opcode, if there is none, we can call InstructionsBuilder.Addtarget and add a nop and attach the jumptarget to it.. This one inside the Instruction is for adding jump target to existing opcode, while the one with the same name AddTarget in the InstructionsBuilder is for situations where we have to set jumptarget to Nop.

I made this change to make it easier to write the code, and make instructoins look shorter.....

Copy link
Member

Choose a reason for hiding this comment

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

But it's working, isn't it?

@Jim8y
Copy link
Contributor Author

Jim8y commented Aug 25, 2024

You also moved some methods and it complicate the review :(

I know,,, i made this super huge pr,,,,,, it just keep growing while i was trying to make it work....... have being working on this for 15 hours without resting......

@Jim8y
Copy link
Contributor Author

Jim8y commented Aug 25, 2024

I tried my best to make it easier for reviewing, and avoided changes to the compile result.....

BUT, there is still a case that will change the artifact,,, which is I added exception message while calling throw for nemuric methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants