Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement IL codes for #91, #93, fix #105 #104
base: devel
Are you sure you want to change the base?
Implement IL codes for #91, #93, fix #105 #104
Changes from 1 commit
7ac3be8
a2c5f30
69cf5f0
e29d09e
e7fef2b
69e814f
663563c
4234755
19c92d9
f6bb406
9091c5b
bab32a0
80e54b5
c5ac124
e01dc9a
4833ba2
90bd0a3
05ececc
f98e142
df35a87
1a9cfe8
2a73440
e8029c1
e1b6cc4
30c0889
08ea06a
ff75c55
5453d39
8e3bac8
e0d75e0
1153f48
b549284
e43b30c
3c3a734
796f113
dd83236
73afbfb
a28587a
92ed390
76eeee7
2b7b595
3cf87a5
bc1476f
9753e44
4790411
ca5706c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cyborgyn The
IsInt32StackFriendlyType
property means "Can place evaluation stack these types, that's compatible int32 type on CLR".Therefore use
IsInt32Type
,IsInt64Type
,IsIntPtrType
andIsUIntPtrType
. By the way, can useTargetType
directly onto decodeContext.PushStack(...) argument.We need regression test for not converter, require these topics:
int32
int64
andIntPtr
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! This way, it will be much nicer. I am getting to know more and more of your code, and how parts are interacting with each other. I need to note, when I uncomment IntPtr unit tests, and also insert those into the IL, a UnitTestFramework fails to discover any tests, even though the produced DLL can be verified to be OK with DotPeek.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cyborgyn Likely
not
opcode's integer type limitation.ECMA-335: III.3.58 shl - shift integer left
section is written:shiftAmount (
si1
) tricky specification, it's exceptedint64
.We need regression test for
shl
andshr
converter, require these topics:shl
/shr
opcode each types int32 int64 and IntPtr combination with shiftAmount by int32 and IntPtr.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have already added quite an amount of tests especially for shr & shr.un, as they were not that intuitive when they sign extend, and when not. I found this:
// Arithmetic Shift right (with sign)
// Warning!!! MSIL always operates for bit storage on Int32, or Int64
// and as it seems, smaller unsigned values (uint8 & uint16) will never
// be able to set the int32 highest bit => they will operate as logical shift right.
// On the other hand: uint32 and uint64 are still operated as arithmetic shift.
Will try to add the native int shift parameter as test, good point, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately I needed to comment them out again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I will take a look on my environment, could you tell me explicit commit id reproducible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it valid on newest commit on branch
feature/implement-ilcodes
(ca5706c) ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented: #104 (comment)