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 function pointer representation #5453

Merged
merged 1 commit into from
Sep 25, 2015
Merged

Fix function pointer representation #5453

merged 1 commit into from
Sep 25, 2015

Conversation

cston
Copy link
Member

@cston cston commented Sep 25, 2015

No description provided.

@cston
Copy link
Member Author

cston commented Sep 25, 2015

@amcasey, @KevinH-MS

@KevinH-MS
Copy link
Contributor

@cston could you clarify why the old code didn't work? It seemed a lot simpler/clearer.

@cston
Copy link
Member Author

cston commented Sep 25, 2015

Previously the tests failed on 64-bit because the values used as placeholders for function pointers were ints rather than pointers. The tests now create actual pointer instances. Other than that, the tests should be equivalent to the previous code.

@KevinH-MS
Copy link
Contributor

Gotcha

cston added a commit that referenced this pull request Sep 25, 2015
Fix function pointer representation
@cston cston merged commit 65bacf8 into dotnet:master Sep 25, 2015
@cston cston deleted the pfn branch September 25, 2015 22:16
var runtime = new DkmClrRuntimeInstance(ReflectionUtilities.GetMscorlibAndSystemCore(GetAssembly(source)), getMemberValue: getMemberValue);
using (runtime.Load())
var assembly = GetUnsafeAssembly(source);
unsafe
Copy link
Contributor

Choose a reason for hiding this comment

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

unsafe unnecessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed, thanks.

@KevinH-MS
Copy link
Contributor

LGTM

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.

3 participants