SI-8185 Correct grammar for single-warning compilation run #3419
Conversation
|
Phrasing chosen to be "1 xxx warning" rather than "a xxx warning" because this also covers inliner warnings. I didn't want to get into (a/an), being grammatically ok-ish is enough. |
| warning("there were %d %s warning(s); re-run with %s for details".format(warnings.size, what, option.name)) | ||
| if (warnings.size > 1) | ||
| warning("there were %d %s warnings; re-run with %s for details".format(warnings.size, what, option.name)) | ||
| else warning("there was one %s warning; re-run with %s for details".format(what, option.name)) |
There was a problem hiding this comment.
Why not use the f-interpolator here?
There was a problem hiding this comment.
Maybe you can tell I spent some time with the f-troop recently.
scala> val size = 5
size: Int = 5
scala> val what = "strange"
what: String = strange
scala> val opt = "demystify"
opt: String = demystify
scala> val x = new Formattable {
| def formatTo(f: Formatter,g:Int,w:Int,p:Int) =
| { val s = if (size>1) s"there were $size $what warnings"
| else s"there was one $what warning"
| f format s }}
x: java.util.Formattable = $anon$1@5e47f5ae
scala> f"$x; re-run with $opt"
res1: String = there were 5 strange warnings; re-run with demystify
|
Thanks, @huitseeker, that looks much nicer. |
|
@scala/team-scala heads up: this PR potentially breaks check files that are in flight in the PR queue, so if we start seeing spurious failures, please rebase on top of this one. It may also cause test failures when merging 2.10.x into master. |
|
[deferring my LGTM], because we may need to update checkfiles of PRs currently in flight. I'll take take care of this after flushing the queue. |
|
PLS REBUILD ALL |
|
(kitty-note-to-self: ignore 35849475) |
|
I'll look into updating the check files once the other PRs are in. PRs like this are fragile unfortunately due to or testing strategy |
|
I can of course squash the two commits if you ping me. But with the intend-to-fix comments I wanted to leave @adriaanm with access to the old commit, which may be more composable w/ work he might have done already. |
|
Thanks for the update. I know you submitted this PR a long time ago, but with our PR queue being so overloaded, we couldn't get to it in time for RC1. I'd still like to generate nicer warnings, but I don't think that qualifies as an RC blocker. I've set the milestone to 2.11.1-RC1, so we'll merge as soon as 2.11.0 goes final. As you've experienced first-hand, it's a time consuming undertaking to update all these check files, so it's certainly not a quick and low-cost fix for some low-hanging fruit. |
|
Sure, no problem. I'll put a thread keeping this one up to date on the backburner. |
|
Actually, @adriaanm, is there a way to pass |
|
@huitseeker aren't those already |
|
@som-snytt I'd have liked to swith from this |
|
PLS REBUILD/pr-scala@2d6ed9 |
|
(kitty-note-to-self: ignore 37184065) |
|
PLS REBUILD/pr-scala@2d6ed9 (github pooped) |
|
(kitty-note-to-self: ignore 37319237) |
|
PLS SYNCH |
|
(kitty-note-to-se
8000
lf: ignore 37354241) |
|
(kitty-note-to-self: ignore 37354241) |
|
(kitty-note-to-self: ignore 37354241) |
|
PLS REBUILD/pr-scala@92d6ed9 |
|
(kitty-note-to-self: ignore 37380185) |
|
Gaah ! I can't figure out what the kitteh does. And the build seems to consistentily trigger a fault in a "git clone" ? I'll just squash and rebuild all. |
|
Maybe because of the network outage at epfl yesterday? |
|
It now went back to an actual check file difference. I suggest just running test/partest --update-check locally. We run the tests under -optimize during PR validation, but that shouldn't influence check files (if it does, the .flags file should include -optimize explicitly to document this and to simplify our workflow) |
|
(You could of course also run |
|
@adriaanm As for "an actual check file difference", it's not the failure you are looking for. Failure of the first commit in the unsquashed version of this PR was expected. The current state of the PR is nothing but the squash of the old one. Labeling tests that depend on optimize is beyond the scope of this PR, alas. I'd suggest trying to run NOLITTER! |
|
(kitty-note-to-self: ignore 37632709) |
|
PLS REBUILD ALL |
|
Could you please resubmit this against the |
A classic mistake of discarding a non-trivial qualifier. We actually should have fixed this before merging scala#3419, as it was raised in review, but I suppose we got too caught up in the difficulty of resolving the right overload of `Symbol_apply` that we forgot.
In Scala 2.8.2, an optimization was added to create a static
cache for Symbol literals (ie, the results of `Symbol.apply("foo"))`.
This saves the map lookup on the second pass through code.
This actually was broken somewhere during the Scala 2.10 series,
after the addition of an overloaded `apply` method to `Symbol`.
The cache synthesis code was made aware of the overload and brought
back to working condition recently, in scala#3419.
However, this has uncovered a latent bug when the Symbol literals are
defined with traits.
One of the enclosed tests failed with:
jvm > t8933b-run.log
java.lang.IllegalAccessError: tried to access field MotherClass.symbol$1 from class MixinWithSymbol$class
at MixinWithSymbol$class.symbolFromTrait(A.scala:3)
at MotherClass.symbolFromTrait(Test.scala:1)
This commit simply disables the optimization if we are in a trait.
Alternative fixes might be: a) make the static Symbol cache field
public / b) "mixin" the static symbol cache. Neither of these
seem worth the effort and risk for an already fairly situational
optimization.
Here's how the optimization looks in a class:
% cat sandbox/test.scala; qscalac sandbox/test.scala && echo ":javap C" | qscala;
class C {
'a; 'b
}
Welcome to Scala version 2.11.5-20141106-145558-aa558dce6d (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_20).
Type in expressions to have them evaluated.
Type :help for more information.
scala> :javap C
Size 722 bytes
MD5 checksum 6bb00189166917254e8d40499ee7c887
Compiled from "test.scala"
public class C
{
public static {};
descriptor: ()V
flags: ACC_PUBLIC, ACC_STATIC
Code:
stack=2, locals=0, args_size=0
0: getstatic #16 // Field scala/Symbol$.MODULE$:Lscala/Symbol$;
3: ldc #18 // String a
5: invokevirtual #22 // Method scala/Symbol$.apply:(Ljava/lang/String;)Lscala/Symbol;
8: putstatic #26 // Field symbol$1:Lscala/Symbol;
11: getstatic #16 // Field scala/Symbol$.MODULE$:Lscala/Symbol$;
14: ldc #28 // String b
16: invokevirtual #22 // Method scala/Symbol$.apply:(Ljava/lang/String;)Lscala/Symbol;
19: putstatic #31 // Field symbol$2:Lscala/Symbol;
22: return
public C();
descriptor: ()V
flags: ACC_PUBLIC
Code:
stack=1, locals=1, args_size=1
0: aload_0
1: invokespecial #34 // Method java/lang/Object."<init>":()V
4: getstatic #26 // Field symbol$1:Lscala/Symbol;
7: pop
8: getstatic #31 // Field symbol$2:Lscala/Symbol;
11: pop
12: return
}
fixup
A classic mistake of discarding a non-trivial qualifier. We actually should have fixed this before merging scala#3419, as it was raised in review, but I suppose we got too caught up in the difficulty of resolving the right overload of `Symbol_apply` that we forgot.
A classic mistake of discarding a non-trivial qualifier. We actually should have fixed this before merging scala#3419, as it was raised in review, but I suppose we got too caught up in the difficulty of resolving the right overload of `Symbol_apply` that we forgot.
50-odd updated tests (!)
review by @dragos or @adriaanm or anybody who isn't too busy (this is an easy one)