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

More thorough treatment of @strictfp. #7356

Merged
merged 1 commit into from
Oct 26, 2018
Merged

More thorough treatment of @strictfp. #7356

merged 1 commit into from
Oct 26, 2018

Conversation

hrhino
Copy link
Member

@hrhino hrhino commented Oct 21, 2018

The JVM accepts ACC_STRICT as a modifier flag on non-abstract methods only. Java itself accepts strictfp on both methods (in which case it applies to the method) and classes (in which case it applies to all methods within the class, as well as nested and inner classes thereof.) Scala has somewhat more ways of nesting methods and classes than Java, so I've extrapolated the rule to be: methods nested inside of a class/module/method definition marked @strictfp are strict.

I also fixed the interaction with value classes: when a method body on a value class was moved to the companion, its @strictfp attribute wasn't moved along with it.

The test case covers nested/inner/local classes and methods, as well as extension methods. I'm leaving specialization to the existing specialization+strictfp tests.

Fixes scala/bug#7954.

@hrhino hrhino added this to the 2.12.8 milestone Oct 21, 2018
@hrhino hrhino requested a review from retronym October 21, 2018 03:53
@SethTisue
Copy link
Member

(targets 2.13.x branch, but you put it on the 2.12.8 milestone?)

@hrhino hrhino changed the base branch from 2.13.x to 2.12.x October 21, 2018 04:03
@hrhino
Copy link
Member Author

hrhino commented Oct 21, 2018

🙀 I should have just let scabot catch my mistake. Thanks!

(Targets 2.12.8 because I'm hopeful. It could also be 2.13, no worries)

(Just getting my toes wet again.)

The JVM accepts `ACC_STRICT` as a modifier flag on non-abstract
methods only. Java itself accepts `strictfp` on both methods
(in which case it applies to the method) and classes (in which
case it applies to all methods within the class, as well as
nested and inner classes thereof.) Scala has somewhat more ways
of nesting methods and classes than Java, so I've extrapolated
the rule to be: methods nested inside of a class/module/method
definition marked `@strictfp` are strict.

I also fixed the interaction with value classes: when a method
body on a value class was moved to the companion, its `@strictfp`
attribute wasn't moved along with it.

The test case covers nested/inner/local classes and methods, as
well as extension methods. I'm leaving specialization to the
existing specialization+strictfp tests.

Fixes scala/bug#7954.
Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

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

Nice, thank you!

@SethTisue
Copy link
Member

@hrhino test failures on Windows at e.g. https://scala-ci.typesafe.com/view/scala-2.12.x/job/scala-2.12.x-integrate-windows/1156/ , this PR seems like the likely culprit

@adriaanm
Copy link
Contributor

it's also failing on Travis. Haven't had a chance yet to see what exactly, but it's in test/files/jvm/strictfp:

Running update-check gives:

--- i/test/files/jvm/strictfp.check
+++ w/test/files/jvm/strictfp.check
@@ -23,8 +23,8 @@ H$I$.bar$10: true
 I.foo: false
 I$.foo: true
 I$.foo$extension: false
-I$.bar$11: false
-I$.bar$12: true
+I$.bar$11: true
+I$.bar$12: false
 J.foo: true
 J$M$1.foo: true
 J$M$1.bar$13: true

does that look right?

@adriaanm
Copy link
Contributor

adriaanm commented Oct 30, 2018

Could perhaps be an interaction with the compiler determinism PR that re-ordered symbols?

@hrhino
Copy link
Member Author

hrhino commented Oct 30, 2018

yikes. evidently I need to start borrowing my girlfriend's windows laptop more often...

The determinism backport PR got merged after this one (dunno why I thought it had been merged before this; sorry).

I'll send in a checkfile fix.

@hrhino
Copy link
Member Author

hrhino commented Oct 30, 2018

#7377

retronym added a commit to retronym/scala that referenced this pull request Nov 6, 2018
Also test that the Java parser doesn't force entry of new symbols
when it parses modifiers that it translates into symbol annotations.

Regressed in scala#7356
retronym added a commit to retronym/scala that referenced this pull request Nov 6, 2018
Also test that the Java parser doesn't force entry of new symbols
when it parses modifiers that it translates into symbol annotations.

Regressed in scala#7356
retronym added a commit to retronym/scala that referenced this pull request Nov 6, 2018
Also test that the Java parser doesn't force entry of new symbols
when it parses modifiers that it translates into symbol annotations.

Regressed in scala#7356
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