Cleanup and refactoring - semicolons, unused or commented out code

mpociecha · mpociecha · commit a8c43dc5caf7 · 2014-12-05T01:21:04.000+01:00
This commit contains some minor changes made by the way when implementing flat classpath. Sample JUnit test that shows that all pieces of JUnit infrastructure work correctly now uses assert method form JUnit as it should do from the beginning. I removed commented out lines which were obvious to me. In the case of less obvious commented out lines I added TODOs as someone should look at such places some day and clean them up. I removed also some unnecessary semicolons and unused imports. Many string concatenations using + have been changed to string interpolation. There's removed unused, private walkIterator method from ZipArchive. It seems that it was unused since this commit: scala@9d4994b However, I had to add an exception for the compatibility checker because it was complaining about this change. I made some trivial corrections/optimisations like use 'findClassFile' method instead of 'findClass' in combination with 'binary' to find the class file.
diff --git a/bincompat-backward.whitelist.conf b/bincompat-backward.whitelist.conf
@@ -203,6 +203,11 @@ filter {
     {
         matchName="scala.reflect.runtime.ThreadLocalStorage#MyThreadLocalStorage.values"
         problemName=MissingMethodProblem
+    },
+    // the below method was the unused private (sic!) method but the compatibility checker was complaining about it
+    {
+        matchName="scala.reflect.io.ZipArchive.scala$reflect$io$ZipArchive$$walkIterator"
+        problemName=MissingMethodProblem
     }
   ]
 }
diff --git a/bincompat-forward.whitelist.conf b/bincompat-forward.whitelist.conf
@@ -301,6 +301,19 @@ filter {
     {
         matchName="scala.reflect.io.FileZipArchive.root"
         problemName=MissingMethodProblem
+    },
+    // introduced the harmless method (instead of the repeated code in several places)
+    {
+        matchName="scala.reflect.runtime.Settings#MultiStringSetting.valueSetByUser"
+        problemName=MissingMethodProblem
+    },
+    {
+        matchName="scala.reflect.runtime.Settings#BooleanSetting.valueSetByUser"
+        problemName=MissingMethodProblem
+    },
+    {
+        matchName="scala.reflect.runtime.Settings#IntSetting.valueSetByUser"
+        problemName=MissingMethodProblem
     }
   ]
 }
diff --git a/src/compiler/scala/tools/nsc/Global.scala b/src/compiler/scala/tools/nsc/Global.scala
@@ -332,7 +332,7 @@ class Global(var currentSettings: Settings, var reporter: Reporter)
           None
       }
 
-    val charset = ( if (settings.encoding.isSetByUser) Some(settings.encoding.value) else None ) flatMap loadCharset getOrElse {
+    val charset = settings.encoding.valueSetByUser flatMap loadCharset getOrElse {
       settings.encoding.value = defaultEncoding // A mandatory charset
       Charset.forName(defaultEncoding)
     }
@@ -347,7 +347,7 @@ class Global(var currentSettings: Settings, var reporter: Reporter)
       }
     }
 
-    ( if (settings.sourceReader.isSetByUser) Some(settings.sourceReader.value) else None ) flatMap loadReader getOrElse {
+    settings.sourceReader.valueSetByUser flatMap loadReader getOrElse {
       new SourceReader(charset.newDecoder(), reporter)
     }
   }
diff --git a/src/compiler/scala/tools/nsc/PhaseAssembly.scala b/src/compiler/scala/tools/nsc/PhaseAssembly.scala
@@ -199,7 +199,7 @@ trait PhaseAssembly {
     // Add all phases in the set to the graph
     val graph = phasesSetToDepGraph(phasesSet)
 
-    val dot = if (settings.genPhaseGraph.isSetByUser) Some(settings.genPhaseGraph.value) else None
+    val dot = settings.genPhaseGraph.valueSetByUser
 
     // Output the phase dependency graph at this stage
     def dump(stage: Int) = dot foreach (n => graphToDotFile(graph, s"$n-$stage.dot"))
diff --git a/src/compiler/scala/tools/nsc/plugins/Plugins.scala b/src/compiler/scala/tools/nsc/plugins/Plugins.scala
@@ -7,7 +7,7 @@
 package scala.tools.nsc
 package plugins
 
-import scala.reflect.io.{ File, Path }
+import scala.reflect.io.Path
 import scala.tools.nsc.util.ClassPath
 import scala.tools.util.PathResolver.Defaults
 
diff --git a/src/compiler/scala/tools/nsc/symtab/SymbolLoaders.scala b/src/compiler/scala/tools/nsc/symtab/SymbolLoaders.scala
@@ -88,8 +88,7 @@ abstract class SymbolLoaders {
       // require yjp.jar at runtime. See SI-2089.
       if (settings.termConflict.isDefault)
         throw new TypeError(
-          root+" contains object and package with same name: "+
-          name+"\none of them needs to be removed from classpath"
+          s"$root contains object and package with same name: $name\none of them needs to be removed from classpath"
         )
       else if (settings.termConflict.value == "package") {
         warning(
@@ -252,7 +251,7 @@ abstract class SymbolLoaders {
    * Load contents of a package
    */
   class PackageLoader(classpath: ClassPath[AbstractFile]) extends SymbolLoader with FlagAgnosticCompleter {
-    protected def description = "package loader "+ classpath.name
+    protected def description = s"package loader ${classpath.name}"
 
     protected def doComplete(root: Symbol) {
       assert(root.isPackageClass, root)
diff --git a/src/compiler/scala/tools/nsc/transform/AddInterfaces.scala b/src/compiler/scala/tools/nsc/transform/AddInterfaces.scala
@@ -351,6 +351,7 @@ abstract class AddInterfaces extends InfoTransform { self: Erasure =>
           while (owner != sym && owner != impl) owner = owner.owner;
           if (owner == impl) This(impl) setPos tree.pos
           else tree
+        //TODO what about this commented out code?
 /* !!!
         case Super(qual, mix) =>
           val mix1 = mix
diff --git a/src/compiler/scala/tools/reflect/ReflectMain.scala b/src/compiler/scala/tools/reflect/ReflectMain.scala
@@ -1,10 +1,10 @@
 package scala.tools
 package reflect
 
+import scala.reflect.internal.util.ScalaClassLoader
 import scala.tools.nsc.Driver
 import scala.tools.nsc.Global
 import scala.tools.nsc.Settings
-import scala.tools.nsc.util.ScalaClassLoader
 import scala.tools.util.PathResolverFactory
 
 object ReflectMain extends Driver {
diff --git a/src/compiler/scala/tools/util/PathResolver.scala b/src/compiler/scala/tools/util/PathResolver.scala
@@ -51,9 +51,8 @@ object PathResolver {
   /** Values found solely by inspecting environment or property variables.
    */
   object Environment {
-    private def searchForBootClasspath = (
+    private def searchForBootClasspath =
       systemProperties find (_._1 endsWith ".boot.class.path") map (_._2) getOrElse ""
-    )
 
     /** Environment variables which java pays attention to so it
      *  seems we do as well.
@@ -107,7 +106,7 @@ object PathResolver {
       else if (scalaLibAsDir.isDirectory) scalaLibAsDir.path
       else ""
 
-    // XXX It must be time for someone to figure out what all these things
+    // TODO It must be time for someone to figure out what all these things
     // are intended to do.  This is disabled here because it was causing all
     // the scala jars to end up on the classpath twice: one on the boot
     // classpath as set up by the runner (or regular classpath under -nobootcp)
@@ -177,7 +176,7 @@ object PathResolver {
   def fromPathString(path: String, context: JavaContext = DefaultJavaContext): JavaClassPath = {
     val s = new Settings()
     s.classpath.value = path
-    new PathResolver(s, context) result
+    new PathResolver(s, context).result
   }
 
   /** With no arguments, show the interesting values in Environment and Defaults.
@@ -263,11 +262,9 @@ abstract class PathResolverBase[BaseClassPathType <: ClassFileLookup[AbstractFil
      *  - Otherwise, if CLASSPATH is set, it is that
      *  - If neither of those, then "." is used.
      */
-    def userClassPath = (
-      if (!settings.classpath.isDefault)
-        settings.classpath.value
+    def userClassPath =
+      if (!settings.classpath.isDefault) settings.classpath.value
       else sys.env.getOrElse("CLASSPATH", ".")
-    )
 
     import classPathFactory._
 
@@ -291,7 +288,7 @@ abstract class PathResolverBase[BaseClassPathType <: ClassFileLookup[AbstractFil
       |  javaBootClassPath    = ${ppcp(javaBootClassPath)}
       |  javaExtDirs          = ${ppcp(javaExtDirs)}
       |  javaUserClassPath    = ${ppcp(javaUserClassPath)}
-      |    useJavaClassPath   = $useJavaClassPath
+      |  useJavaClassPath     = $useJavaClassPath
       |  scalaBootClassPath   = ${ppcp(scalaBootClassPath)}
       |  scalaExtDirs         = ${ppcp(scalaExtDirs)}
       |  userClassPath        = ${ppcp(userClassPath)}
diff --git a/src/interactive/scala/tools/nsc/interactive/Global.scala b/src/interactive/scala/tools/nsc/interactive/Global.scala
@@ -128,8 +128,8 @@ class Global(settings: Settings, _reporter: Reporter, projectName: String = "")
     else NullLogger
 
   import log.logreplay
-  debugLog("logger: " + log.getClass + " writing to " + (new java.io.File(logName)).getAbsolutePath)
-  debugLog("classpath: "+classPath)
+  debugLog(s"logger: ${log.getClass} writing to ${(new java.io.File(logName)).getAbsolutePath}")
+  debugLog(s"classpath: $classPath")
 
   private var curTime = System.nanoTime
   private def timeStep = {
diff --git a/src/partest-extras/scala/tools/partest/BytecodeTest.scala b/src/partest-extras/scala/tools/partest/BytecodeTest.scala
@@ -116,10 +116,8 @@ abstract class BytecodeTest {
       sys.error(s"Didn't find method '$name' in class '${classNode.name}'")
 
   protected def loadClassNode(name: String, skipDebugInfo: Boolean = true): ClassNode = {
-    val classBytes: InputStream = (for {
-      classRep <- classpath.findClass(name)
-      binary <- classRep.binary
-    } yield binary.input) getOrElse sys.error(s"failed to load class '$name'; classpath = $classpath")
+    val classBytes: InputStream = classpath.findClassFile(name).map(_.input)
+      .getOrElse(sys.error(s"failed to load class '$name'; classpath = $classpath"))
 
     val cr = new ClassReader(classBytes)
     val cn = new ClassNode()
diff --git a/src/reflect/scala/reflect/internal/SymbolTable.scala b/src/reflect/scala/reflect/internal/SymbolTable.scala
@@ -338,7 +338,6 @@ abstract class SymbolTable extends macros.Universe
       case _ => false
     }
     if (pkgModule.isModule && !fromSource) {
-      // println("open "+pkgModule)//DEBUG
       openPackageModule(pkgModule, pkgClass)
     }
   }
diff --git a/src/reflect/scala/reflect/internal/Symbols.scala b/src/reflect/scala/reflect/internal/Symbols.scala
@@ -181,7 +181,7 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
     type AccessBoundaryType = Symbol
     type AnnotationType     = AnnotationInfo
 
-    // TODO - don't allow names to be renamed in this unstructured a fashion.
+    // TODO - don't allow names to be renamed in this unstructured fashion.
     // Rename as little as possible.  Enforce invariants on all renames.
     type TypeOfClonedSymbol >: Null <: Symbol { type NameType = Symbol.this.NameType }
 
@@ -1461,11 +1461,9 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
     def info: Type = try {
       var cnt = 0
       while (validTo == NoPeriod) {
-        //if (settings.debug.value) System.out.println("completing " + this);//DEBUG
         assert(infos ne null, this.name)
         assert(infos.prev eq null, this.name)
         val tp = infos.info
-        //if (settings.debug.value) System.out.println("completing " + this.rawname + tp.getClass());//debug
 
         if ((_rawflags & LOCKED) != 0L) { // rolled out once for performance
           lock {
@@ -1474,6 +1472,7 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
           }
         } else {
           _rawflags |= LOCKED
+          // TODO another commented out lines - this should be solved in one way or another
 //          activeLocks += 1
  //         lockedSyms += this
         }
diff --git a/src/reflect/scala/reflect/internal/settings/MutableSettings.scala b/src/reflect/scala/reflect/internal/settings/MutableSettings.scala
@@ -31,6 +31,9 @@ abstract class MutableSettings extends AbsSettings {
       v = arg
       postSetHook()
     }
+
+    /** Returns Some(value) in the case of a value set by user and None otherwise. */
+    def valueSetByUser: Option[T] = if (isSetByUser) Some(value) else None
   }
 
   def Xexperimental: BooleanSetting
diff --git a/src/reflect/scala/reflect/io/VirtualFile.scala b/src/reflect/scala/reflect/io/VirtualFile.scala
@@ -75,10 +75,10 @@ class VirtualFile(val name: String, override val path: String) extends AbstractF
   }
 
   /** Does this abstract file denote an existing file? */
-  def create() { unsupported() }
+  def create(): Unit = unsupported()
 
   /** Delete the underlying file or directory (recursively). */
-  def delete() { unsupported() }
+  def delete(): Unit = unsupported()
 
   /**
    * Returns the abstract file in this abstract directory with the
diff --git a/src/reflect/scala/reflect/io/ZipArchive.scala b/src/reflect/scala/reflect/io/ZipArchive.scala
@@ -74,19 +74,14 @@ abstract class ZipArchive(override val file: JFile) extends AbstractFile with Eq
   def container = unsupported()
   def absolute  = unsupported()
 
-  private def walkIterator(its: Iterator[AbstractFile]): Iterator[AbstractFile] = {
-    its flatMap { f =>
-      if (f.isDirectory) walkIterator(f.iterator)
-      else Iterator(f)
-    }
-  }
   /** ''Note:  This library is considered experimental and should not be used unless you know what you are doing.'' */
   sealed abstract class Entry(path: String) extends VirtualFile(baseName(path), path) {
     // have to keep this name for compat with sbt's compiler-interface
     def getArchive: ZipFile = null
     override def underlyingSource = Some(self)
     override def toString = self.path + "(" + path + ")"
   }
+
   /** ''Note:  This library is considered experimental and should not be used unless you know what you are doing.'' */
   class DirEntry(path: String) extends Entry(path) {
     val entries = mutable.HashMap[String, Entry]()
@@ -245,11 +240,9 @@ final class ManifestResources(val url: URL) extends ZipArchive(null) {
     val manifest = new Manifest(input)
     val iter     = manifest.getEntries().keySet().iterator().filter(_.endsWith(".class")).map(new ZipEntry(_))
 
-    while (iter.hasNext) {
-      val zipEntry = iter.next()
+    for (zipEntry <- iter) {
       val dir = getDir(dirs, zipEntry)
-      if (zipEntry.isDirectory) dir
-      else {
+      if (!zipEntry.isDirectory) {
         class FileEntry() extends Entry(zipEntry.getName) {
           override def lastModified = zipEntry.getTime()
           override def input        = resourceInputStream(path)
@@ -285,14 +278,14 @@ final class ManifestResources(val url: URL) extends ZipArchive(null) {
   private def resourceInputStream(path: String): InputStream = {
     new FilterInputStream(null) {
       override def read(): Int = {
-        if(in == null) in = Thread.currentThread().getContextClassLoader().getResourceAsStream(path);
+        if(in == null) in = Thread.currentThread().getContextClassLoader().getResourceAsStream(path)
         if(in == null) throw new RuntimeException(path + " not found")
-        super.read();
+        super.read()
       }
 
       override def close(): Unit = {
-        super.close();
-        in = null;
+        super.close()
+        in = null
       }
     }
   }
diff --git a/src/repl/scala/tools/nsc/interpreter/IMain.scala b/src/repl/scala/tools/nsc/interpreter/IMain.scala
@@ -1282,9 +1282,11 @@ object IMain {
 
     def getProgram(statements: String*): String = null
 
-    def getScriptEngine: ScriptEngine = new IMain(this, new Settings() {
-      usemanifestcp.value = true
-    })
+    def getScriptEngine: ScriptEngine = {
+      val settings = new Settings()
+      settings.usemanifestcp.value = true
+      new IMain(this, settings)
+    }
   }
 
   // The two name forms this is catching are the two sides of this assignment:
diff --git a/src/scalap/scala/tools/scalap/Arguments.scala b/src/scalap/scala/tools/scalap/Arguments.scala
@@ -9,7 +9,7 @@
 package scala.tools.scalap
 
 import scala.collection.mutable
-import mutable.{ Buffer, ListBuffer }
+import mutable.ListBuffer
 
 object Arguments {
   case class Parser(optionPrefix: Char) {
@@ -47,7 +47,7 @@ object Arguments {
     }
 
     def parseBinding(str: String, separator: Char): (String, String) = (str indexOf separator) match {
-      case -1   => argumentError("missing '" + separator + "' in binding '" + str + "'") ; ("", "")
+      case -1   => argumentError(s"missing '$separator' in binding '$str'") ; ("", "")
       case idx  => ((str take idx).trim, (str drop (idx + 1)).trim)
     }
 
@@ -71,19 +71,19 @@ object Arguments {
             i += 1
           } else if (optionalArgs contains args(i)) {
             if ((i + 1) == args.length) {
-              argumentError("missing argument for '" + args(i) + "'")
+              argumentError(s"missing argument for '${args(i)}'")
               i += 1
             } else {
               res.addArgument(args(i), args(i + 1))
               i += 2
             }
           } else if (optionalBindings contains args(i)) {
             if ((i + 1) == args.length) {
-              argumentError("missing argument for '" + args(i) + "'")
+              argumentError(s"missing argument for '${args(i)}'")
               i += 1
             } else {
               res.addBinding(args(i),
-                parseBinding(args(i + 1), optionalBindings(args(i))));
+                parseBinding(args(i + 1), optionalBindings(args(i))))
               i += 2
             }
           } else {
@@ -92,23 +92,23 @@ object Arguments {
             while ((i == j) && iter.hasNext) {
               val prefix = iter.next
               if (args(i) startsWith prefix) {
-                res.addPrefixed(prefix, args(i).substring(prefix.length()).trim());
+                res.addPrefixed(prefix, args(i).substring(prefix.length()).trim())
                 i += 1
               }
             }
             if (i == j) {
-              val iter = prefixedBindings.keysIterator;
+              val iter = prefixedBindings.keysIterator
               while ((i == j) && iter.hasNext) {
                 val prefix = iter.next
                 if (args(i) startsWith prefix) {
                   val arg = args(i).substring(prefix.length()).trim()
                   i = i + 1
                   res.addBinding(prefix,
-                    parseBinding(arg, prefixedBindings(prefix)));
+                    parseBinding(arg, prefixedBindings(prefix)))
                 }
               }
               if (i == j) {
-                argumentError("unknown option '" + args(i) + "'")
+                argumentError(s"unknown option '${args(i)}'")
                 i = i + 1
               }
             }
@@ -119,7 +119,7 @@ object Arguments {
 
   def parse(options: String*)(args: Array[String]): Arguments = {
     val parser = new Parser('-')
-    options foreach (parser withOption _)
+    options foreach parser.withOption
     parser parse args
   }
 }
@@ -142,7 +142,7 @@ class Arguments {
     if (key.length > 0)
       bindings.getOrElseUpdate(tag, new mutable.HashMap)(key) = value
 
-  def addBinding(tag: String, binding: Tuple2[String, String]): Unit =
+  def addBinding(tag: String, binding: (String, String)): Unit =
     addBinding(tag, binding._1, binding._2)
 
   def addOther(arg: String): Unit = others += arg
diff --git a/src/scalap/scala/tools/scalap/Main.scala b/src/scalap/scala/tools/scalap/Main.scala
diff --git a/test/junit/scala/tools/nsc/SampleTest.scala b/test/junit/scala/tools/nsc/SampleTest.scala
diff --git a/test/junit/scala/tools/nsc/symtab/SymbolTableForUnitTesting.scala b/test/junit/scala/tools/nsc/symtab/SymbolTableForUnitTesting.scala
diff --git a/test/script-tests/README b/test/script-tests/README

Original file line number	Diff line number	Diff line change
`@@ -203,6 +203,11 @@ filter {`
`203`	`203`	`{`
`204`	`204`	`matchName="scala.reflect.runtime.ThreadLocalStorage#MyThreadLocalStorage.values"`
`205`	`205`	`problemName=MissingMethodProblem`
	`206`	`+ },`
	`207`	`+ // the below method was the unused private (sic!) method but the compatibility checker was complaining about it`
	`208`	`+ {`
	`209`	`+ matchName="scala.reflect.io.ZipArchive.scala$reflect$io$ZipArchive$$walkIterator"`
	`210`	`+ problemName=MissingMethodProblem`
`206`	`211`	`}`
`207`	`212`	`]`
`208`	`213`	`}`
Original file line number	Diff line number	Diff line change
`@@ -301,6 +301,19 @@ filter {`
`301`	`301`	`{`
`302`	`302`	`matchName="scala.reflect.io.FileZipArchive.root"`
`303`	`303`	`problemName=MissingMethodProblem`
	`304`	`+ },`
	`305`	`+ // introduced the harmless method (instead of the repeated code in several places)`
	`306`	`+ {`
	`307`	`+ matchName="scala.reflect.runtime.Settings#MultiStringSetting.valueSetByUser"`
	`308`	`+ problemName=MissingMethodProblem`
	`309`	`+ },`
	`310`	`+ {`
	`311`	`+ matchName="scala.reflect.runtime.Settings#BooleanSetting.valueSetByUser"`
	`312`	`+ problemName=MissingMethodProblem`
	`313`	`+ },`
	`314`	`+ {`
	`315`	`+ matchName="scala.reflect.runtime.Settings#IntSetting.valueSetByUser"`
	`316`	`+ problemName=MissingMethodProblem`
`304`	`317`	`}`
`305`	`318`	`]`
`306`	`319`	`}`
Original file line number	Diff line number	Diff line change
`@@ -332,7 +332,7 @@ class Global(var currentSettings: Settings, var reporter: Reporter)`
`332`	`332`	`None`
`333`	`333`	`}`
`334`	`334`
`335`		`- val charset = ( if (settings.encoding.isSetByUser) Some(settings.encoding.value) else None ) flatMap loadCharset getOrElse {`
	`335`	`+ val charset = settings.encoding.valueSetByUser flatMap loadCharset getOrElse {`
`336`	`336`	`settings.encoding.value = defaultEncoding // A mandatory charset`
`337`	`337`	`Charset.forName(defaultEncoding)`
`338`	`338`	`}`
`@@ -347,7 +347,7 @@ class Global(var currentSettings: Settings, var reporter: Reporter)`
`347`	`347`	`}`
`348`	`348`	`}`
`349`	`349`
`350`		`- ( if (settings.sourceReader.isSetByUser) Some(settings.sourceReader.value) else None ) flatMap loadReader getOrElse {`
	`350`	`+ settings.sourceReader.valueSetByUser flatMap loadReader getOrElse {`
`351`	`351`	`new SourceReader(charset.newDecoder(), reporter)`
`352`	`352`	`}`
`353`	`353`	`}`