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

Code Cleanup: Java 16 instanceof pattern matching in pde.core #712

Merged
merged 2 commits into from
Sep 20, 2023

Conversation

alshamams
Copy link
Contributor

The commit tries to implement a code cleanup in pattern matching using instanceof operator according to Java 16 guidelines, making the code more concise.
The change has been applied in org.eclipse.pde.core project.

Kindly review @akurtakov @iloveeclipse @vik-chand @gireeshpunathil and do let me know in case of any edits.

@github-actions
Copy link

github-actions bot commented Aug 17, 2023

Test Results

     270 files  ±0       270 suites  ±0   1h 0m 58s ⏱️ - 6m 56s
  3 334 tests ±0    3 304 ✔️ +1  30 💤 ±0  0 ±0 
10 299 runs  ±0  10 209 ✔️ +1  90 💤 ±0  0 ±0 

Results for commit 01fca40. ± Comparison against base commit 86ad78d.

♻️ This comment has been updated with latest results.

if (pluginBase instanceof IFragment) {
// host specification
IFragment fragment = (IFragment) pluginBase;
if (pluginBase instanceof IFragment fragment) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@akurtakov
Copy link
Member

@alshamams It's good to open a bug against jdt for removed comments.
@jjohnstn Would you please take a look at why this cleanup doesn't keep comments?

@HannesWell
Copy link
Member

@alshamams It's good to open a bug against jdt for removed comments.

IIRC some other JDT clean-ups like 'add braces' has a similar issue for years (if not decades), nevertheless it would be great if this is fixed.

@jjohnstn
Copy link
Contributor

@alshamams It's good to open a bug against jdt for removed comments. @jjohnstn Would you please take a look at why this cleanup doesn't keep comments?

This is a no-win area for cleanups. The compiler treats a comment preceding a statement as belonging to the statement. When a statement is moved, it's comments generally go along and when removed, it's comments are removed. There is a way to preserve a comment but this isn't done for removals because there is no way to know if the comment really belongs to the statement or is a general logic comment like the ones you have above. It can never get it right not knowing the context. I was thinking of adding an option to preserve comments on remove but this is a large-scale undertaking which might require changes to all existing cleanups. I think in this particular case, defaulting to keeping the comment might be reasonable since the cast statement is a trivial operation not usually worth documenting. I will open an enhancement issue.

@gireeshpunathil
Copy link
Contributor

There is a way to preserve a comment but this isn't done for removals because there is no way to know if the comment really belongs to the statement or is a general logic comment like the ones you have above. It can never get it right not knowing the context.

@jjohnstn - is the code removal logic generic for all the clean-up functions? or is it specific to clean-up of instanceof optimisation? at least in this (instanceof) case it is reasonable to assert that the comment is not associated with the trivial casting, instead the code that follows. So I would retain the comment by default.

@jjohnstn
Copy link
Contributor

There is a way to preserve a comment but this isn't done for removals because there is no way to know if the comment really belongs to the statement or is a general logic comment like the ones you have above. It can never get it right not knowing the context.

@jjohnstn - is the code removal logic generic for all the clean-up functions? or is it specific to clean-up of instanceof optimisation? at least in this (instanceof) case it is reasonable to assert that the comment is not associated with the trivial casting, instead the code that follows. So I would retain the comment by default.

@gireeshpunathil As I noted, the comment for the cast statement is unlikely to be a comment regarding the cast. I posted #752 so the comment is no longer removed. It has been merged for 4.30 M1.

@akurtakov
Copy link
Member

Thanks!

@akurtakov akurtakov merged commit f8a1dc3 into eclipse-pde:master Sep 20, 2023
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.

5 participants