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

SI-7515 Emit final in bytecode for final inner classes. #2636

Closed
wants to merge 1 commit into from

Conversation

retronym
Copy link
Member

@retronym retronym commented Jun 6, 2013

As we did before a regression in 18efded / SI-5676.

This commit tightens up the condition in which the FINAL
modifier is ommitted; it now only does this for the module
classes of nested objects.

Review by @JamesIry

As we did before a regression in 18efded / SI-5676.

This commit tightens up the condition in which the FINAL
modifier is ommitted; it now *only* does this for the module
classes of nested objects.
@retronym
Copy link
Member Author

retronym commented Jun 6, 2013

Submitting against 2.10.x; this was a regression from 2.9.2 to 2.10.0, so it would be nice to deliver the fix in 2.10.3.

@JamesIry
Copy link
Contributor

JamesIry commented Jun 6, 2013

I think this will be LGTM, but I want to make triple sure we're not concerned about binary compatibility issues.

In particular this does create a binary incompatibility but one that will never be seen by people writing Scala. But what about people writing Java, could they have extended a final inner class that was missing the final attribute in its classfile? Do we care?

@adriaanm, @paulp, thoughts?

@soc
Copy link
Member

soc commented Jun 7, 2013

If we ever cared about usage from Java, we wouldn't have sealed ...

@paulp
Copy link
Contributor

paulp commented Jun 7, 2013

@soc sealed is a compile-time feature which can't be enforced from java. This has literally zero bearing on how to assess binary-incompatible changes which can be observed from java.

@paulp
Copy link
Contributor

paulp commented Jun 7, 2013

It is abundantly non-obvious how this PR relates to SI-7515. Oh, it's SI-7151.

@paulp
Copy link
Contributor

paulp commented Jun 7, 2013

I am against a change like this which impacts binary compatibility, especially in the absence of any compelling motivation. If we were generating final too frequently, that would be one thing; it is harder to break things by relaxing restrictions than by tightening them. I think this should be master only.

@retronym retronym closed this Jun 7, 2013
@retronym
Copy link
Member Author

retronym commented Jun 7, 2013

Reopened against master as #2635. And fixed my JIRA dyslexia to boot.

inkytonik added a commit to inkytonik/kiama that referenced this pull request Jun 24, 2018
…iting

This check doesn?t work on Scala 2.10 due to the bug that is addressed by the following change:

scala/scala#2636

The change wasn?t back-ported to 2.10 due to binary compatibility concerns. So we now don?t
require a value that is being unboxed to be of a final class. This should be ok since we only
unbox when a primitive argument type is expected which (we think) should only happen if
the argument is an AnyVal.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants