TODO cleanups based on @havocp's comments.

compile/src/main/scala/sbt/compiler/javac/DiagnosticsReporter.scala

@@ -11,14 +11,16 @@ import xsbti.{ Severity, Reporter }
  * @param reporter
  */
 final class DiagnosticsReporter(reporter: Reporter) extends DiagnosticListener[JavaFileObject] {
-
+  val END_OF_LINE_MATCHER = "[\r\n]|[\r]|[\n]"
+  val EOL = System.getProperty("line.separator")
   private def fixedDiagnosticMessage(d: Diagnostic[_ <: JavaFileObject]): String = {
     def getRawMessage = d.getMessage(null)
     def fixWarnOrErrorMessage = {
       val tmp = getRawMessage
       // we fragment off the line/source/type report from the message.
+      // NOTE - End of line handling may be off.
       val lines: Seq[String] =
-        tmp.split("[\r\n]") match {
+        tmp.split(END_OF_LINE_MATCHER) match {
           case Array(head, tail @ _*) =>
             val newHead = head.split(":").last
             newHead +: tail
@@ -26,8 +28,7 @@ final class DiagnosticsReporter(reporter: Reporter) extends DiagnosticListener[J
             head.split(":").last :: Nil
           case Array() => Seq.empty[String]
         }
-      // TODO - Real EOL
-      lines.mkString("\n")
+      lines.mkString(EOL)
     }
     d.getKind match {
       case Diagnostic.Kind.ERROR | Diagnostic.Kind.WARNING | Diagnostic.Kind.MANDATORY_WARNING => fixWarnOrErrorMessage
@@ -38,7 +39,9 @@ final class DiagnosticsReporter(reporter: Reporter) extends DiagnosticListener[J
     try Option(source).map(_.toUri.normalize).map(new File(_)).map(_.getAbsolutePath)
     catch {
       case t: IllegalArgumentException =>
-        // Oracle JDK6 has a super dumb notion of what a URI is
+        // Oracle JDK6 has a super dumb notion of what a URI is.  In fact, it's not even a legimitate URL, but a dump
+        // of the filename in a "I hope this works to toString it" kind of way.  This appears to work in practice
+        // but we may need to re-evaluate.
         Option(source).map(_.toUri.toString)
     }
   }
@@ -56,7 +59,9 @@ final class DiagnosticsReporter(reporter: Reporter) extends DiagnosticListener[J
           Logger.o2m(if (d.getLineNumber == -1) None
           else Option(new Integer(d.getLineNumber.toInt)))
         override def lineContent = {
-          // TODO - Is this pulling error correctly? Is null an ok return value?
+          // TODO - Is this pulling contents of the line correctly?
+          // Would be ok to just return null if this version of the JDK doesn't support grabbing
+          // source lines?
           Option(d.getSource).
             flatMap(s => Option(s.getCharContent(true))).
             map(_.subSequence(d.getStartPosition.intValue, d.getEndPosition.intValue).toString).

compile/src/main/scala/sbt/compiler/javac/JavaCompilerAdapter.scala

@@ -16,7 +16,8 @@ import xsbti.compile.{ MultipleOutput, SingleOutput, Output }
  */
 class JavaCompilerAdapter(delegate: NewJavaTool, scalaInstance: xsbti.compile.ScalaInstance, cpOptions: xsbti.compile.ClasspathOptions) extends xsbti.compile.JavaCompiler {
   override final def compile(sources: Array[File], classpath: Array[File], output: Output, options: Array[String], log: xsbti.Logger): Unit = {
-    // TODO - 5 max errors ok?
+    // TODO - 5 max errors ok?  We're not expecting this code path to be called, ever.  This is only for clients who try to use the xsbti.compile.JavaCompiler interface
+    // outside of the incremental compiler, for some reason.
     val reporter = new LoggerReporter(5, log)
     compileWithReporter(sources, classpath, output, options, reporter, log)
   }
@@ -26,11 +27,11 @@ class JavaCompilerAdapter(delegate: NewJavaTool, scalaInstance: xsbti.compile.Sc
       case mo: MultipleOutput => throw new RuntimeException("Javac doesn't support multiple output directories")
     }
     val args = commandArguments(Seq(), classpath, target, options, log)
-    // TODO - is sorting the sources correct here?
+    // We sort the sources for deterministic results.
     val success = delegate.run(sources.sortBy(_.getAbsolutePath), args)(log, reporter)
-    // TODO - What should this error message be, and how do we ensure things are correct here?
     if (!success) {
-      // TODO - Should we track only the problems from this javac?
+      // TODO - Will the reporter have problems from Scalac?  It appears like it does not, only from the most recent run.
+      // This is because the incremental compiler will not run javac if scalac fails.
       throw new CompileFailed(args.toArray, "javac returned nonzero exit code", reporter.problems())
     }
   }

compile/src/main/scala/sbt/compiler/javac/JavacProcessLogger.scala

@@ -89,7 +89,7 @@ object JavaNoPosition extends Position {
 
 /** A wrapper around xsbti.Problem with java-specific options. */
 final case class JavaProblem(val position: Position, val severity: Severity, val message: String) extends xsbti.Problem {
-  override def category: String = null // TODO - what is this even supposed to be?
+  override def category: String = "javac" // TODO - what is this even supposed to be?  For now it appears unused.
   override def toString = s"$severity @ $position - $message"
 }
 
@@ -98,6 +98,7 @@ class JavaErrorParser(relativeDir: File) extends util.parsing.combinator.RegexPa
   // Here we track special handlers to catch "Note:" and "Warning:" lines.
   private val NOTE_LINE_PREFIXES = Array("Note: ", "\u6ce8: ", "\u6ce8\u610f\uff1a ")
   private val WARNING_PREFIXES = Array("warning", "\u8b66\u544a", "\u8b66\u544a\uff1a")
+  private val END_OF_LINE = System.getProperty("line.separator")
 
   override val skipWhitespace = false
 
@@ -110,6 +111,7 @@ class JavaErrorParser(relativeDir: File) extends util.parsing.combinator.RegexPa
   }
   // Parses the rest of an input line.
   val restOfLine: Parser[String] =
+    // TODO - Can we use END_OF_LINE here without issues?
     allUntilChars(Array('\n', '\r')) ~ "[\r]?[\n]?".r ^^ {
       case msg ~ _ => msg
     }
@@ -151,9 +153,14 @@ class JavaErrorParser(relativeDir: File) extends util.parsing.combinator.RegexPa
     } catch {
       case e: NumberFormatException => false
     }
+
+  // Helper to extract an integer from a string
+  private object ParsedInteger {
+    def unapply(s: String): Option[Int] = try Some(Integer.parseInt(s)) catch { case e: NumberFormatException => None }
+  }
   // Parses a line number
   val line: Parser[Int] = allUntilChar(':') ^? {
-    case x if isInteger(x) => Integer.parseInt(x)
+    case ParsedInteger(x) => x
   }
   val allUntilCharat: Parser[String] = allUntilChar('^')
 

compile/src/main/scala/sbt/compiler/javac/LocalJava.scala

@@ -47,6 +47,8 @@ final class LocalJavadoc() extends NewJavadoc {
     try {
       exitCode = LocalJava.unsafeJavadoc(allArguments.toArray, warnOrError, warnOrError, infoWriter)
     } finally {
+      warnOrError.close()
+      infoWriter.close()
       javacLogger.flush(exitCode)
     }
     // We return true or false, depending on success.

compile/src/main/scala/sbt/compiler/javac/ProcessLoggerWriter.scala

@@ -4,10 +4,7 @@ import sbt.{ Level, ProcessLogger }
 
 /** Delegates a stream into a process logger. Mimics LoggerWriter, but for the ProcessLogger interface which differs. */
 private class ProcessLoggerWriter(delegate: ProcessLogger, level: Level.Value, nl: String = System.getProperty("line.separator")) extends java.io.Writer {
-
   private[this] val buffer = new StringBuilder
-  private[this] val lines = new collection.mutable.ListBuffer[String]
-
   override def close() = flush()
   override def flush(): Unit =
     synchronized {
@@ -16,12 +13,6 @@ private class ProcessLoggerWriter(delegate: ProcessLogger, level: Level.Value, n
         buffer.clear()
       }
     }
-  def flushLines(level: Level.Value): Unit =
-    synchronized {
-      for (line <- lines)
-        log(line)
-      lines.clear()
-    }
   override def write(content: Array[Char], offset: Int, length: Int): Unit =
     synchronized {
       buffer.appendAll(content, offset, length)