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

Removal of region constructor with handle param #1309

Merged

Conversation

amartya4256
Copy link
Contributor

@amartya4256 amartya4256 commented Jun 26, 2024

This commit removes the constructor and the following method win32_new and hereby confirm that none of the clients consume them.

contributes to #62 and #127

@amartya4256 amartya4256 requested a review from niraj-modi as a code owner June 26, 2024 13:28
@amartya4256 amartya4256 force-pushed the region_remove_constructor branch from f37f672 to 1c60efc Compare June 26, 2024 13:29
Copy link
Contributor

github-actions bot commented Jun 26, 2024

Test Results

   470 files  ±0     470 suites  ±0   7m 41s ⏱️ -3s
 4 135 tests ±0   4 127 ✅ ±0   8 💤 ±0  0 ❌ ±0 
16 336 runs  ±0  16 244 ✅ ±0  92 💤 ±0  0 ❌ ±0 

Results for commit 7e19604. ± Comparison against base commit bd223bb.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

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

The removals looks good to me. Only the commit message seems a little broken: "[...] and the following method win_32 [...]". @amartya4256 Would be great if you could fix that.

As an additional note on this PR: is does not only simplify the existing code, it will also simplify and improve the Windows HiDPI improvements for the Region class (#1278).

Even though the removed static method is tagged with @noreference (and even only part of an OS-specific implementation) and should thus be safe to remove, it would be great to have this confirmed by someone more experienced. Maybe @akurtakov you can give an advice whether this removal is okay?

@akurtakov
Copy link
Member

The removals looks good to me. Only the commit message seems a little broken: "[...] and the following method win_32 [...]". Would be great if you could fix that.

As an additional note on this PR: is does not only simplify the existing code, it will also simplify and improve the Windows HiDPI improvements for the Region class (#1278).

Even though the removed static method is tagged with @noreference (and even only part of an OS-specific implementation) and should thus be safe to remove, it would be great to have this confirmed by someone more experienced. Maybe @akurtakov you can give an advice whether this removal is okay?

This removal is fine as this method is only available on windows, has been properly annotated with noreference and will actually break cross platformness of the codebase if anyone has used it and even that would be quite hard as one would have to know the C pointer in order to use it proper.

@HeikoKlare
Copy link
Contributor

Thank you for the quick response and confirmation!

@amartya4256 amartya4256 force-pushed the region_remove_constructor branch from 1c60efc to 85c35de Compare June 26, 2024 15:17
This commit removes the constructor and the following method win32_new and
hereby confirms that none of the clients consume them.

contributes to eclipse-platform#62 and eclipse-platform#127
@amartya4256 amartya4256 force-pushed the region_remove_constructor branch from 85c35de to 7e19604 Compare June 26, 2024 15:18
@HeikoKlare HeikoKlare merged commit 69184cc into eclipse-platform:master Jun 26, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants