SI-8185 Correct grammar for single-warning compilation run #3419

Closed
wants to merge 1 commit into
from

Projects

None yet

6 participants

@huitseeker
Contributor

50-odd updated tests (!)

review by @dragos or @adriaanm or anybody who isn't too busy (this is an easy one)

@huitseeker
Contributor

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.

@soc soc and 1 other commented on an outdated diff Jan 28, 2014
src/compiler/scala/tools/nsc/Global.scala
@@ -1164,7 +1164,9 @@ class Global(var currentSettings: Settings, var reporter: Reporter)
else if (!(warnings contains pos)) warnings += ((pos, msg))
def summarize() =
if (warnings.nonEmpty && (option.isDefault || settings.fatalWarnings))
- 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))
@soc
soc Jan 28, 2014 Member

Why not use the f-interpolator here?

@som-snytt
som-snytt Jan 28, 2014 Contributor

Or the s-interpolator.

@som-snytt
som-snytt Jan 28, 2014 Contributor

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
@adriaanm adriaanm self-assigned this Feb 5, 2014
@adriaanm
Member
adriaanm commented Feb 5, 2014

Thanks, @huitseeker, that looks much nicer.

@adriaanm
Member
adriaanm commented Feb 5, 2014

@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.

@adriaanm
Member
adriaanm commented Feb 5, 2014

[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.

@adriaanm adriaanm added the on-hold label Feb 10, 2014
@adriaanm
Member

PLS REBUILD ALL

@scala-jenkins

(kitty-note-to-self: ignore 35849475)
๐Ÿฑ Roger! Rebuilding pr-scala for ac15b06. ๐Ÿšจ

@huitseeker huitseeker added needs-attention and removed tested labels Feb 24, 2014
@adriaanm
Member

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

@adriaanm adriaanm modified the milestone: 2.11.1-RC1, 2.11.0-RC1 Feb 25, 2014
@huitseeker
Contributor

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.

@adriaanm
Member

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.

@huitseeker
Contributor

Sure, no problem. I'll put a thread keeping this one up to date on the backburner.

@huitseeker
Contributor

Actually, @adriaanm, is there a way to pass partest options from ant ? That would help with maintaining this one. Just a --show-diff would be tremendously helpful.

@som-snytt
Contributor

@huitseeker aren't those already diff *.log *.check at the end of the build output? If that's what you mean. I don't think you can pass arbitrary command line opts.

@huitseeker
Contributor

@som-snytt I'd have liked to swith from this ant-generated diff to a partest-generated --show-diff (more readable). And have the ability to do an --update-check, or --verbose ...

@huitseeker
Contributor

The failure on 26e6848 is expected and will disappear after squash. The one on 92d6ed9 is a network issue, unrelated to this PR.

@huitseeker
Contributor

PLS REBUILD/pr-scala@2d6ed9

@scala-jenkins

(kitty-note-to-self: ignore 37184065)
๐Ÿฑ Roger! Rebuilding pr-scala for 2d6ed9. ๐Ÿšจ

@huitseeker
Contributor

PLS REBUILD/pr-scala@2d6ed9

(github pooped)

@scala-jenkins

(kitty-note-to-self: ignore 37319237)
๐Ÿฑ Roger! Rebuilding pr-scala for 2d6ed9. ๐Ÿšจ

@huitseeker
Contributor

PLS SYNCH
NOLITTER!
PLS REBUILD/pr-scala@92d6ed9

@scala-jenkins

(kitty-note-to-self: ignore 37354241)
๐Ÿฑ Roger! Rebuilding pr-scala for 2d6ed9. ๐Ÿšจ

@scala-jenkins

(kitty-note-to-self: ignore 37354241)
๐Ÿฑ Synchronaising! ๐Ÿ™

@scala-jenkins

(kitty-note-to-self: ignore 37354241)
๐Ÿฑ cleaning up... sorry! ๐Ÿฑ

@scala-jenkins

(kitty-note-to-self: ignore 37380185)
๐Ÿฑ Roger! Rebuilding pr-scala for 92d6ed9. ๐Ÿšจ

@huitseeker
Contributor

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.

@adriaanm
Member

Maybe because of the network outage at epfl yesterday?

@adriaanm
Member

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)

@adriaanm
Member

(You could of course also run SCALAC_OPTS=-optimize test/partest --update-check)

@huitseeker huitseeker added tested and removed needs-attention labels Mar 13, 2014
@huitseeker
Contributor

@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 SCALAC_OPTS=-optimize test/partest to get a sense of the task.

NOLITTER!

@scala-jenkins

(kitty-note-to-self: ignore 37632709)
๐Ÿฑ cleaning up... sorry! ๐Ÿฑ

@retronym
Member
retronym commented May 7, 2014

PLS REBUILD ALL

@retronym
Member
retronym commented May 9, 2014

Could you please resubmit this against the 2.11.x branch? I am retiring the master branch today. Sorry about the long and circuitous path that this PR has taken, but I'm sure that correct grammar will prevail.

@retronym retronym closed this May 9, 2014
@som-snytt
Contributor
@adriaanm adriaanm removed this from the 2.11.1 milestone May 19, 2014
@retronym retronym added a commit to retronym/scala that referenced this pull request Nov 6, 2014
@retronym retronym SI-7974 Fix over-eager optimization of Symbol literals
A classic mistake of discarding a non-trivial qualifier.

We actually should have fixed this before merging #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.
7dfa516
@retronym retronym added a commit to retronym/scala that referenced this pull request Nov 6, 2014
@retronym retronym SI-8933 Disable static Symbol literal cache in traits
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 #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
4a8a5c1
@retronym retronym added a commit to retronym/scala that referenced this pull request Nov 6, 2014
@retronym retronym SI-7974 Fix over-eager optimization of Symbol literals
A classic mistake of discarding a non-trivial qualifier.

We actually should have fixed this before merging #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.
d7a8f53
@retronym retronym added a commit to retronym/scala that referenced this pull request Nov 7, 2014
@retronym retronym SI-7974 Fix over-eager optimization of Symbol literals
A classic mistake of discarding a non-trivial qualifier.

We actually should have fixed this before merging #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.
2078f79
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment