From 6c18e37886e90d217579112ccf867c22658273be Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Mon, 14 Mar 2016 10:02:16 +0100 Subject: Address reviewer comments. --- src/dotty/tools/dotc/ast/Desugar.scala | 4 +-- src/dotty/tools/dotc/ast/NavigateAST.scala | 2 +- src/dotty/tools/dotc/ast/Positioned.scala | 30 ++++++++++++++--------- src/dotty/tools/dotc/parsing/Parsers.scala | 4 ++- src/dotty/tools/dotc/rewrite/Rewrites.scala | 31 +++++++++--------------- src/dotty/tools/dotc/transform/LazyVals.scala | 5 +++- src/dotty/tools/dotc/typer/Typer.scala | 6 ++--- src/dotty/tools/dotc/typer/VarianceChecker.scala | 4 +-- 8 files changed, 46 insertions(+), 40 deletions(-) (limited to 'src') diff --git a/src/dotty/tools/dotc/ast/Desugar.scala b/src/dotty/tools/dotc/ast/Desugar.scala index 2fb1b3ef5..7a305a8b8 100644 --- a/src/dotty/tools/dotc/ast/Desugar.scala +++ b/src/dotty/tools/dotc/ast/Desugar.scala @@ -500,7 +500,7 @@ object desugar { * val/var/lazy val p = e * * Otherwise, in case there is exactly one variable x_1 in pattern - * val/var/lazy val p = e ==> val/var x_1 = (e: @unchecked) match (case p => (x_1)) + * val/var/lazy val p = e ==> val/var/lazy val x_1 = (e: @unchecked) match (case p => (x_1)) * * in case there are zero or more than one variables in pattern * val/var/lazy p = e ==> private synthetic [lazy] val t$ = (e: @unchecked) match (case p => (x_1, ..., x_N)) @@ -508,7 +508,7 @@ object desugar { * ... * val/var/def x_N = t$._N * If the original pattern variable carries a type annotation, so does the corresponding - * ValDef. + * ValDef or DefDef. */ def makePatDef(mods: Modifiers, pat: Tree, rhs: Tree)(implicit ctx: Context): Tree = pat match { case VarPattern(named, tpt) => diff --git a/src/dotty/tools/dotc/ast/NavigateAST.scala b/src/dotty/tools/dotc/ast/NavigateAST.scala index 3753c6702..782866bad 100644 --- a/src/dotty/tools/dotc/ast/NavigateAST.scala +++ b/src/dotty/tools/dotc/ast/NavigateAST.scala @@ -47,7 +47,7 @@ object NavigateAST { } case _ => untypedPath(tree.pos) match { - case path @ last :: _ if last.pos == tree.pos || !exactMatch => path + case (path @ last :: _) if last.pos == tree.pos || !exactMatch => path case _ => Nil } } diff --git a/src/dotty/tools/dotc/ast/Positioned.scala b/src/dotty/tools/dotc/ast/Positioned.scala index cff77c919..e7f5de591 100644 --- a/src/dotty/tools/dotc/ast/Positioned.scala +++ b/src/dotty/tools/dotc/ast/Positioned.scala @@ -54,14 +54,23 @@ abstract class Positioned extends DotClass with Product { */ private[dotc] def setPosUnchecked(pos: Position) = curPos = pos - /** If any children of this node do not have positions, set them to the given position, + /** If any children of this node do not have positions, + * fit their positions between the positions of the known subtrees * and transitively visit their children. + * The method is likely time-critical because it is invoked on any node + * we create, so we want to avoid object allocations in the common case. + * The method is naturally expressed as two mutually (tail-)recursive + * functions, one which computes the next element to consider or terminates if there + * is none and the other which propagates the position information to that element. + * But since mutual tail recursion is not supported in Scala, we express it instead + * as a while loop with a termination by return in the middle. */ private def setChildPositions(pos: Position): Unit = { - var n = productArity - var elems: List[Any] = Nil - var end = pos.end - var outstanding: List[Positioned] = Nil + var n = productArity // subnodes are analyzed right to left + var elems: List[Any] = Nil // children in lists still to be considered, from right to left + var end = pos.end // the last defined offset, fill in positions up to this offset + var outstanding: List[Positioned] = Nil // nodes that need their positions filled once a start position + // is known, from left to right. def fillIn(ps: List[Positioned], start: Int, end: Int): Unit = ps match { case p :: ps1 => p.setPos(Position(start, end)) @@ -69,20 +78,20 @@ abstract class Positioned extends DotClass with Product { case nil => } while (true) { - var nextElem: Any = null + var nextChild: Any = null // the next child to be considered if (elems.nonEmpty) { - nextElem = elems.head + nextChild = elems.head elems = elems.tail } else if (n > 0) { n = n - 1 - nextElem = productElement(n) + nextChild = productElement(n) } else { fillIn(outstanding, pos.start, end) return } - nextElem match { + nextChild match { case p: Positioned => if (p.pos.exists) { fillIn(outstanding, p.pos.end, end) @@ -91,8 +100,7 @@ abstract class Positioned extends DotClass with Product { } else outstanding = p :: outstanding case xs: List[_] => - val newElems = xs.reverse - elems = if (elems.isEmpty) newElems else newElems ::: elems + elems = elems ::: xs.reverse case _ => } } diff --git a/src/dotty/tools/dotc/parsing/Parsers.scala b/src/dotty/tools/dotc/parsing/Parsers.scala index aa2058692..6ec75a8b2 100644 --- a/src/dotty/tools/dotc/parsing/Parsers.scala +++ b/src/dotty/tools/dotc/parsing/Parsers.scala @@ -1763,7 +1763,9 @@ object Parsers { */ def defDefOrDcl(mods: Modifiers): Tree = atPos(tokenRange) { def scala2ProcedureSyntax(resultTypeStr: String) = { - val toInsert = if (in.token == LBRACE) s"$resultTypeStr =" else ": Unit " + val toInsert = + if (in.token == LBRACE) s"$resultTypeStr =" + else ": Unit " // trailing space ensures that `def f()def g()` works. testScala2Mode(s"Procedure syntax no longer supported; `$toInsert' should be inserted here") && { patch(source, Position(in.lastOffset), toInsert) true diff --git a/src/dotty/tools/dotc/rewrite/Rewrites.scala b/src/dotty/tools/dotc/rewrite/Rewrites.scala index cda9e8565..7ab0e5d59 100644 --- a/src/dotty/tools/dotc/rewrite/Rewrites.scala +++ b/src/dotty/tools/dotc/rewrite/Rewrites.scala @@ -51,7 +51,7 @@ object Rewrites { def writeBack(): Unit = { val out = source.file.output - val chars = apply(source.content) + val chars = apply(source.underlying.content) val bytes = new String(chars).getBytes out.write(bytes) out.close() @@ -62,28 +62,21 @@ object Rewrites { * given by `pos` in `source` by `replacement` */ def patch(source: SourceFile, pos: Position, replacement: String)(implicit ctx: Context): Unit = - ctx.settings.rewrite.value match { - case Some(rewrites: Rewrites) => - rewrites.patched.get(source) match { - case Some(ps) => - ps.addPatch(pos, replacement) - case None => - rewrites.patched(source) = new Patches(source) - patch(source, pos, replacement) - } - case _ => - } + for (rewrites <- ctx.settings.rewrite.value) + rewrites.patched + .getOrElseUpdate(source, new Patches(source)) + .addPatch(pos, replacement) + + /** Patch position in `ctx.compilationUnit.source`. */ + def patch(pos: Position, replacement: String)(implicit ctx: Context): Unit = + patch(ctx.compilationUnit.source, pos, replacement) /** If -rewrite is set, apply all patches and overwrite patched source files. */ def writeBack()(implicit ctx: Context) = - ctx.settings.rewrite.value match { - case Some(rewrites: Rewrites) => - for (source <- rewrites.patched.keys) { - ctx.println(s"[patched file ${source.file.path}]") - rewrites.patched(source).writeBack() - } - case _ => + for (rewrites <- ctx.settings.rewrite.value; source <- rewrites.patched.keys) { + ctx.println(s"[patched file ${source.file.path}]") + rewrites.patched(source).writeBack() } } diff --git a/src/dotty/tools/dotc/transform/LazyVals.scala b/src/dotty/tools/dotc/transform/LazyVals.scala index 50db7b3fb..fc02e68cc 100644 --- a/src/dotty/tools/dotc/transform/LazyVals.scala +++ b/src/dotty/tools/dotc/transform/LazyVals.scala @@ -70,7 +70,10 @@ class LazyVals extends MiniPhaseTransform with IdentityDenotTransformer with Nee val isField = sym.owner.isClass if (isField) { if (sym.isVolatile || - (sym.is(Flags.Module)/* || ctx.scala2Mode*/) && !sym.is(Flags.Synthetic)) // TODO assume @voliat + (sym.is(Flags.Module)/* || ctx.scala2Mode*/) && + // TODO assume @volatile once LazyVals uses static helper constructs instead of + // ones in the companion object. + !sym.is(Flags.Synthetic)) // module class is user-defined. // Should be threadsafe, to mimic safety guaranteed by global object transformMemberDefVolatile(tree) diff --git a/src/dotty/tools/dotc/typer/Typer.scala b/src/dotty/tools/dotc/typer/Typer.scala index 1a0525c6d..2069e790b 100644 --- a/src/dotty/tools/dotc/typer/Typer.scala +++ b/src/dotty/tools/dotc/typer/Typer.scala @@ -998,7 +998,7 @@ class Typer extends Namer with TypeAssigner with Applications with Implicits wit if (sym.is(Lazy, butNot = Deferred | Module | Synthetic) && !sym.isVolatile && ctx.scala2Mode && ctx.settings.rewrite.value.isDefined && !ctx.isAfterTyper) - patch(ctx.compilationUnit.source, Position(toUntyped(vdef).envelope.start), "@volatile ") + patch(Position(toUntyped(vdef).envelope.start), "@volatile ") } def typedDefDef(ddef: untpd.DefDef, sym: Symbol)(implicit ctx: Context) = track("typedDefDef") { @@ -1158,8 +1158,8 @@ class Typer extends Namer with TypeAssigner with Applications with Implicits wit if (ctx.scala2Mode) { // Under -rewrite, patch `x _` to `(() => x)` ctx.migrationWarning(msg, tree.pos) - patch(ctx.compilationUnit.source, Position(tree.pos.start), "(() => ") - patch(ctx.compilationUnit.source, Position(qual.pos.end, tree.pos.end), ")") + patch(Position(tree.pos.start), "(() => ") + patch(Position(qual.pos.end, tree.pos.end), ")") res = typed(untpd.Function(Nil, untpd.TypedSplice(res))) } else ctx.error(msg, tree.pos) diff --git a/src/dotty/tools/dotc/typer/VarianceChecker.scala b/src/dotty/tools/dotc/typer/VarianceChecker.scala index 1769835da..274218ee3 100644 --- a/src/dotty/tools/dotc/typer/VarianceChecker.scala +++ b/src/dotty/tools/dotc/typer/VarianceChecker.scala @@ -114,8 +114,8 @@ class VarianceChecker()(implicit ctx: Context) { case Some(VarianceError(tvar, required)) => def msg = i"${varianceString(tvar.flags)} $tvar occurs in ${varianceString(required)} position in type ${sym.info} of $sym" if (ctx.scala2Mode && sym.owner.isConstructor) { - ctx.migrationWarning(s"According to new variance rules, this is no longer accepted; need to annotate with @uncheckedVariance:\n$msg, pos = ${sym.pos}", sym.pos) - patch(ctx.compilationUnit.source, Position(pos.end), " @scala.annotation.unchecked.uncheckedVariance") // TODO use an import or shorten if possible + ctx.migrationWarning(s"According to new variance rules, this is no longer accepted; need to annotate with @uncheckedVariance:\n$msg", sym.pos) + patch(Position(pos.end), " @scala.annotation.unchecked.uncheckedVariance") // TODO use an import or shorten if possible } else ctx.error(msg, sym.pos) case None => -- cgit v1.2.3