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
Limit unsafe code to NET451 #654
Conversation
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.
Thanks @prmathur-microsoft .
Is it possible to completely remove these files from ns1.3/ns2.0?
unsafe class ScheduledOverlapped | ||
|
||
#else | ||
class ScheduledOverlapped |
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.
Do we need this class in anything but NET451?
Overlapped operations are always unsafe
. Can't we just remove the file from ns13/ns20?
} | ||
[Fx.Tag.SecurityNote(Critical = "Provides a set of unsafe methods used to work with the WaitableTimer")] | ||
[SecurityCritical] | ||
static class TimerHelper | ||
{ | ||
public static unsafe SafeWaitHandle CreateWaitableTimer() | ||
public static SafeWaitHandle CreateWaitableTimer() |
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.
This function is using UnsafeNativeMethods so it must be marked as unsafe
.
@@ -672,7 +662,7 @@ public static unsafe SafeWaitHandle CreateWaitableTimer() | |||
} | |||
return handle; | |||
} | |||
public static unsafe long Set(SafeWaitHandle timer, long dueTime) | |||
public static long Set(SafeWaitHandle timer, long dueTime) |
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.
Same as above.
unsafe class ScheduledOverlapped | ||
#else | ||
class ScheduledOverlapped |
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.
Same as above - class contains unsafe code. It should be marked as unsafe
. It would be ideal if we can remove it from ns builds.
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 limited the unsafe code to net451 and hence changing the class to unsafe only for net451.
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.
The nature of the class (performing IOCP / Overlapped I/O) is unsafe. If all unsafe references have been removed then the class should not be doing anything.
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.
We'll leave this as-is for now and remove the unused code when we remove net451 support.
One unrelated file upload test failed. |
Checklist
master
branch.Description of the changes
Reference/Link to the issue solved with this PR (if any)