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 11 commits
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)
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 Is this operator valid negative? (Equals method is same as?)
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.
since String implements IComparable, it's not a ReferenceEquals compare anymore defined on Object. It has 3 possible return values of -1, 0, 1, where 0 indicates match. In case the two strings xname and yname don't match, we do a second try, with explicit interface naming match. There were cases observed, where the Equals specifically was used, and even though it should have been match, it didn't get to the point to validate a parameter list, and returned false. The same logic was adapted to the sorting aid, Compare() method.
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 look to seem this expression makes short circuit evaluation:
At (a) xname value and yname value is different (not 0), and contains string of last part at (b-1) and (b-2), so final result at (c) is always different...? (always not 0)
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.
Think about the following scenario:
The trouble was, for the class MethodInformation.Name was "NS.Ib.Method()", but in the interface definition it was just "Method()". So when searching for interface implementation on the class, it didn't find it. One of them's DeclaringType is the class, the other's is the interface. So, the Equals() always returned false, and in the CallConverters.cs, at line 100 it throw Exception at this place:
So, what this thing does, is just give an other chance, putting the full "namespace.interfaceTypeName." before the appropriate method name (x, or y), and does an other compare. A side effect in the case for ordering, is that all interface implementation would be packed together after each other, I guess, but it is not used for things like this, as I am aware. In the case of Equals() there is no side effect.
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 was insight it and finally understood what is problem for ;)
See branch
feature/implement-ilcodes-r1
/ 19bb2eaThere was nothing regression test for this code fragment, it's my badness....
The callvirt opcode devirtualization function try it a few conditions:
At (a), this expression will test arg0 (this) is
class
type (analysis by IL2C), but this case couldn't be realize when using C# compiler. (Maybe) C# compiler never generate these conditions.So I added some test written IL (b159b49, 65ebbbe). These test will cause making (a) condition with true and false. And I understood your explain.
Then I thought of a way to make your method better. In this case, we can only check for method matches by symbolic name, only methods on class/value type.
For example, if the F# compiler generate an explicit interface implementation method, the method names will not necessarily match. To make sure that the methods match exactly, check if they are included in
IMethodInformation.Overrides
property.So, I made the following changes to make sure the unit test passes.