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 : modernize switch expressions #934

Merged
merged 2 commits into from
Dec 12, 2023

Conversation

elsazac
Copy link
Member

@elsazac elsazac commented Dec 12, 2023

Code cleanup : modernize switch expressions

@elsazac elsazac requested a review from lshanmug as a code owner December 12, 2023 11:25
Copy link
Contributor

github-actions bot commented Dec 12, 2023

Test Results

     299 files  ±0       299 suites  ±0   6m 15s ⏱️ +43s
  4 096 tests ±0    4 088 ✔️ ±0    8 💤 ±0  0 ±0 
12 200 runs  ±0  12 127 ✔️ ±0  73 💤 ±0  0 ±0 

Results for commit 89377d9. ± Comparison against base commit 7c78245.

♻️ This comment has been updated with latest results.

Comment on lines 1673 to 1674
NSNumber styleObj = null;
switch (style) {
case SWT.UNDERLINE_SINGLE:
styleObj = NSNumber.numberWithInt(OS.kAXUnderlineStyleSingle);
break;
case SWT.UNDERLINE_DOUBLE:
styleObj = NSNumber.numberWithInt(OS.kAXUnderlineStyleDouble);
break;
case SWT.UNDERLINE_SQUIGGLE:
styleObj = switch (style) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could those 2 lines be joined?

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed the changes.

Comment on lines 1718 to 1720
int osAlignment = 0;
// FIXME: Doesn't account for right-to-left text?
switch (docAttributes.alignment) {
case SWT.CENTER:
osAlignment = OS.NSTextAlignmentCenter;
break;
case SWT.RIGHT:
osAlignment = OS.NSTextAlignmentRight;
break;
case SWT.LEFT:
default:
osAlignment = OS.NSTextAlignmentLeft;
break;
}
osAlignment = switch (docAttributes.alignment) {
Copy link
Contributor

Choose a reason for hiding this comment

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

same

Copy link
Member Author

Choose a reason for hiding this comment

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

addresses the changes

Comment on lines 253 to 263
newCursor = switch (cursorOrientation) {
case SWT.UP -> new Cursor(display, SWT.CURSOR_SIZENS);
case SWT.DOWN -> new Cursor(display, SWT.CURSOR_SIZENS);
case SWT.LEFT -> new Cursor(display, SWT.CURSOR_SIZEWE);
case SWT.RIGHT -> new Cursor(display, SWT.CURSOR_SIZEWE);
case SWT.LEFT | SWT.UP -> new Cursor(display, SWT.CURSOR_SIZENWSE);
case SWT.RIGHT | SWT.DOWN -> new Cursor(display, SWT.CURSOR_SIZENWSE);
case SWT.LEFT | SWT.DOWN -> new Cursor(display, SWT.CURSOR_SIZENESW);
case SWT.RIGHT | SWT.UP -> new Cursor(display, SWT.CURSOR_SIZENESW);
default -> new Cursor(display, SWT.CURSOR_SIZEALL);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be something like

Cursor newCursor = new Cursor(display, switch (cursorOrientation) {
  case SWT.UP, SWT.DOWN -> SWT.CURSOR_SIZENS;
  case SWT.LEFT, SWT.RIGHT -> SWT.CURSOR_SIZEWE;
  case (SWT.LEFT | SWT.UP), (SWT.RIGHT | SWT.DOWN ) -> new Cursor(display, SWT.CURSOR_SIZENWSE);
...
});

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed the changes.

Comment on lines 254 to 255
case NEAREST -> imageData.scaledTo (scaledWidth, scaledHeight);
default -> imageData.scaledTo (scaledWidth, scaledHeight);
Copy link
Contributor

Choose a reason for hiding this comment

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

default should be sufficient t cover both cases

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed the changes

Comment on lines 2466 to 2467
case JCS_GRAYSCALE:
yield 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not case JCS_GRAYSCALE -> 1; ?

Copy link
Member Author

Choose a reason for hiding this comment

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

when I applied your changes I was getting this error "mixing of different case semantics is not allowed". I converted all the cases into one single format. Not sure if I did it right. Can you suggest?

Copy link
Contributor

Choose a reason for hiding this comment

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

the -> syntax must be used on all fields; it's not allowed to mix case X: ... and case Y -> ...

Copy link
Member Author

Choose a reason for hiding this comment

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

I have addressed that way.

Comment on lines 239 to 240
OutputStream os = null;
switch (loader.compression) {
case 0:
os = new DeflaterOutputStream(baos, new Deflater(NO_COMPRESSION));
break;
case 1:
os = new DeflaterOutputStream(baos, new Deflater(BEST_SPEED));
break;
case 3:
os = new DeflaterOutputStream(baos, new Deflater(BEST_COMPRESSION));
break;
default:
os = new DeflaterOutputStream(baos, new Deflater(DEFAULT_COMPRESSION));
break;
}
os = switch (loader.compression) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Those lines could be joined,

Copy link
Contributor

Choose a reason for hiding this comment

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

Or even better

OutputStream os = new DeflaterOutputStream(baos, switch (loader.compression) {
case 0 -> NO_COMPRESSION;
case 1 -> BEST_SPEED;
case 3 -> BEST_COMPRESSION;
default -> DEFAULT_COMPRESSION;
}

Copy link
Member Author

@elsazac elsazac Dec 12, 2023

Choose a reason for hiding this comment

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

addressed the changes.

@mickaelistria mickaelistria merged commit d54d964 into eclipse-platform:master Dec 12, 2023
13 checks passed
@mickaelistria
Copy link
Contributor

Thank you!

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.

2 participants