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

Merged

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

   285 files  ±0     285 suites  ±0   47m 45s ⏱️ - 3m 25s
 3 586 tests ±0   3 510 ✅ ±0   76 💤 ±0  0 ❌ ±0 
10 950 runs  ±0  10 719 ✅ ±0  231 💤 ±0  0 ❌ ±0 

Results for commit 483f516. ± Comparison against base commit 0e399fc.

♻️ 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

@jukzi jukzi force-pushed the Unnecessary_SuppressWarningsdeprecation branch from 44528ee to 483f516 Compare January 16, 2025 13:59
@jukzi
Copy link
Contributor Author

jukzi commented Jan 16, 2025

ignoring unrelated warning "The method RegistryFilteredTree.updateToolbar(boolean) overrides a method from FilteredTree that has been deprecated since version 2025-03 and marked for removal"

@jukzi jukzi merged commit 1897bc7 into eclipse-pde:master Jan 16, 2025
15 of 18 checks passed
@jukzi jukzi deleted the Unnecessary_SuppressWarningsdeprecation branch January 16, 2025 15:12
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