Skip to content

Code Quality: Replaced DllImport calls with CsWin32 generations for event object operations #17086

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Lamparter
Copy link
Contributor

Resolved / Related Issues

@yaira2
Copy link
Member

yaira2 commented Apr 30, 2025

@Lamparter can you please fill out the template with the steps used to validate these changes?

Copy link
Member

@0x5bfa 0x5bfa left a comment

Choose a reason for hiding this comment

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

Otherwise, LGTM.

@Lamparter
Copy link
Contributor Author

@0x5bfa requesting your review

});

_ = CoWaitForMultipleObjects(
_ = Win32PInvoke.CoWaitForMultipleObjects(
Copy link
Member

Choose a reason for hiding this comment

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

Pretty sure you should pin the array.

{
fixed (char* fileDialog = "FILEDIALOG")
{
HANDLE eventHandle = PInvoke.CreateEvent(bManualReset: false, bInitialState: false, lpName: fileDialog);
Copy link
Member

Choose a reason for hiding this comment

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

Can't you just:

Suggested change
HANDLE eventHandle = PInvoke.CreateEvent(bManualReset: false, bInitialState: false, lpName: fileDialog);
HANDLE eventHandle = PInvoke.CreateEvent((SECURITY_ATTRIBUTES*)null, false, false, fileDialog);

Win32PInvoke.CloseHandle(eventHandle);
unsafe
{
fixed (char* fileDialog = "FILEDIALOG")
Copy link
Member

Choose a reason for hiding this comment

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

pszEventObjectName please.

{
fixed (char* fileDialog = "FILEDIALOG")
{
HANDLE eventHandle = PInvoke.CreateEvent(bManualReset: false, bInitialState: false, lpName: fileDialog);
Copy link
Member

Choose a reason for hiding this comment

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

Also hEventHandle please.

{
IntPtr eventHandle = CreateEvent(IntPtr.Zero, true, false, null);
HANDLE eventHandle = PInvoke.CreateEvent(bManualReset: true, bInitialState: false, lpName: null);
Copy link
Member

Choose a reason for hiding this comment

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

As above.

{
IntPtr eventHandle = CreateEvent(IntPtr.Zero, true, false, null);
HANDLE eventHandle = PInvoke.CreateEvent(bManualReset: true, bInitialState: false, lpName: null);
Copy link
Member

Choose a reason for hiding this comment

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

As above.

@@ -2124,7 +2125,7 @@ private void WatchForDirectoryChanges(string path, CloudDriveSyncStatus syncStat
notifyFilters |= FILE_NOTIFY_CHANGE_ATTRIBUTES;

var overlapped = new OVERLAPPED();
overlapped.hEvent = CreateEvent(IntPtr.Zero, false, false, null);
overlapped.hEvent = PInvoke.CreateEvent(null, false, false, null).DangerousGetHandle();
Copy link
Member

Choose a reason for hiding this comment

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

As above.

@@ -2235,7 +2236,7 @@ private void WatchForGitChanges()
var notifyFilters = FILE_NOTIFY_CHANGE_DIR_NAME | FILE_NOTIFY_CHANGE_FILE_NAME | FILE_NOTIFY_CHANGE_LAST_WRITE | FILE_NOTIFY_CHANGE_SIZE | FILE_NOTIFY_CHANGE_CREATION;

var overlapped = new OVERLAPPED();
overlapped.hEvent = CreateEvent(IntPtr.Zero, false, false, null);
overlapped.hEvent = PInvoke.CreateEvent(null, false, false, null).DangerousGetHandle();
Copy link
Member

Choose a reason for hiding this comment

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

Asa above.

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