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 Unnecessary @SuppressWarnings("deprecation") #1564

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jukzi
Copy link
Contributor

@jukzi jukzi commented Jan 14, 2025

No description provided.

@akurtakov
Copy link
Member

Suppressing "removal" will get the nasty effect that no one will see any warning until it's actually removed and be "surpised" that build broke.
@HannesWell Are you fine with removing the usage of these deprecated for removal APIs?

@jukzi
Copy link
Contributor Author

jukzi commented Jan 14, 2025

Suppressing "removal" will get the nasty effect that no one will see any warning until it's actually removed and be "surpised" that build broke.

You would only see such warning if you have pde in the workspace. In that case you will get a hard compilation error if you remove the implementation.

@laeubi
Copy link
Contributor

laeubi commented Jan 14, 2025

Suppressing "removal" will get the nasty effect that no one will see any warning until it's actually removed and be "surpised" that build broke.

You would only see such warning if you have pde in the workspace. In that case you will get a hard compilation error if you remove the implementation.

If it is PDE internally using deprecated PDE thing it should better be fixed than suppressed (or itself deprecated).

@jukzi jukzi force-pushed the Unnecessary_SuppressWarningsdeprecation branch from 4ea9a13 to a16f999 Compare January 14, 2025 15:54
@akurtakov
Copy link
Member

IMO, usage of Platform.ARCH_X86 should be removed altogether now that Platform marked it for removal now rather than fixing yet another I-build failure in 2 years.

@laeubi
Copy link
Contributor

laeubi commented Jan 14, 2025

A good alternative is to simply inline the constant to these places.

@HannesWell
Copy link
Member

IMO, usage of Platform.ARCH_X86 should be removed altogether now that Platform marked it for removal now rather than fixing yet another I-build failure in 2 years.

The affected part of the code is for the Product Editor and it's ability to add arch-specific arguments. If we remove these code paths one cannot target older versions of the Eclipse-runtime with a recent development Eclipse.

A good alternative is to simply inline the constant to these places.

I would also be in favor of this approach as it's simple doesn't add much overhead and extends the compatibility of PDE with older Eclipse releases to target.

Copy link

github-actions bot commented Jan 14, 2025

Test Results

   279 files   -   6     279 suites   - 6   46m 37s ⏱️ - 3m 29s
 3 586 tests ±  0   3 509 ✅  -   1   76 💤 ± 0  1 ❌ +1 
10 756 runs   - 194  10 553 ✅  - 166  202 💤  - 29  1 ❌ +1 

For more details on these failures, see this check.

Results for commit 44528ee. ± Comparison against base commit aaaa0c0.

♻️ This comment has been updated with latest results.

@jukzi jukzi force-pushed the Unnecessary_SuppressWarningsdeprecation branch from a16f999 to 44528ee Compare January 15, 2025 07:16
@jukzi
Copy link
Contributor Author

jukzi commented Jan 15, 2025

A good alternative is to simply inline the constant to these places.

I would also be in favor of this approach as it's simple doesn't add much overhead and extends the compatibility of PDE with older Eclipse releases to target.

inlined ARCH_X86

@SuppressWarnings("deprecation")
private final String[] COMBO_ARCHLABELS = new String[] { PDEUIMessages.PropertiesSection_All, Platform.ARCH_X86,
private final String[] COMBO_ARCHLABELS = new String[] { PDEUIMessages.PropertiesSection_All,
IArgumentsInfo.ARCH_X86,
Copy link
Contributor

Choose a reason for hiding this comment

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

What makes me wonder if we should add the other supported archs now here? so what about arm, longaarch and alike?

Copy link
Contributor

@laeubi laeubi left a comment

Choose a reason for hiding this comment

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

Looks fine to me

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.

4 participants