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

Restore pre-1.0.0 behavior for codepoint funs and ObjectEqualityComparator #199

Merged
merged 2 commits into from
Sep 20, 2024

Conversation

lppedd
Copy link
Contributor

@lppedd lppedd commented Sep 7, 2024

After making ObjectEqualityComparator an object, I have noticed the outputted code (and the code that's actually invoked) changed.

See the diff. Left 1.0.0-RC4 / Right 1.0.0

image

So in 1.0.0 a call to ObjectEqualityComparator.equals goes through:

f2s(a, b)
  y30(a, b)
   equals(a, b)

Compared to just

f2s(a, b)
  equals(a, b)

In RC4. There is also that weird:

var tmp = (a == null ? true : !(a == null)) ? a : THROW_CCE();

which I don't understand. Why would a ClassCastException be thrown there?
So I thought that it's better to revert it.

@lppedd
Copy link
Contributor Author

lppedd commented Sep 7, 2024

This closes #198.

Copy link
Member

@ftomassetti ftomassetti left a comment

Choose a reason for hiding this comment

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

I understand the equals operation being invoked on the INSTANCE field, but I am not sure about removing constants. In my opinion they made the code simpler

internal const val MIN_LOW_SURROGATE: Int = 0xDC00
internal const val HIGH_SURROGATE_ENCODE_OFFSET: Int =
(MIN_HIGH_SURROGATE - (MIN_SUPPLEMENTARY_CODE_POINT_ ushr 10))
// Note(Edoardo): kept as named constant as it is a complex operation.
Copy link
Member

Choose a reason for hiding this comment

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

To me, this seems to make the code less readable than before

} else {
1
}
if (codePoint >= 0x10000 /* MIN_SUPPLEMENTARY_CODE_POINT */) 2 else 1
Copy link
Member

Choose a reason for hiding this comment

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

I think removing braces is good, but the constant was making the code easier to read and maintain

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ftomassetti Regarding constants, I agree that keeping them as const val makes the code more readable.
My choice of manually inlining them comes from the fact an internal const val used outside of the file boundary produces additional accessors.

For example

// A.kt
internal const val MIN_LOW_SURROGATE: Int = 0xDC00

// B.kt
if (x > MIN_LOW_SURROGATE) { }

translates to

var MIN_LOW_SURROGATE = 56320;

function getMIN_LOW_SURROGATE() {
  return MIN_LOW_SURROGATE;
}

if (x > getMIN_LOW_SURROGATE()) { }

instead of

if (x > 56320) { }

It is a subjective decision, so I'm up to reverting it, but it has its benefits.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the late reply. Yes, I would prefer you to revert it. I see your point, but I would favor readability over the slight performance hit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ftomassetti done. I've only removed the inline modifier, so that the compiler can at least partially inline constants.

This way constants are automatically inlined for those functions.
Plus it results in less JS code.
@lppedd
Copy link
Contributor Author

lppedd commented Sep 17, 2024

You might have to manually restart the workflow.

@ftomassetti
Copy link
Member

Thank you @lppedd jobs restarted. If they pass I will merge

@ftomassetti ftomassetti merged commit 79e955b into Strumenta:master Sep 20, 2024
6 checks passed
@lppedd lppedd deleted the refactor/inlines branch September 20, 2024 07:23
@lppedd
Copy link
Contributor Author

lppedd commented Sep 20, 2024

@ftomassetti thanks. Would it be ok to release a 1.0.1?

@ftomassetti
Copy link
Member

@lppedd I just released 1.0.1

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