Review.

Mostly GREEN, some special parts RED.
refs 862

org.fortiss.af3.tools/trunk/src/org/fortiss/af3/tools/base/SimpleToolRunnerBase.java

@@ -29,7 +29,7 @@ import org.fortiss.tooling.kernel.model.INamedElement;
 
 /**
  * Base implementation of {@link ToolRunnerBase} without intermediate generation capability.
- * Instead of generating the input file(s) for the external tool this runner uses an existing file
+ * Instead of generating the input file(s) for the external tool, this runner uses an existing file
  * (or folder) as its source.
  * 
  * The result type of the runner is its console output, i.e., a list of strings.
@@ -37,7 +37,7 @@ import org.fortiss.tooling.kernel.model.INamedElement;
  * @author hoelzl
  * @author $Author$
  * @version $Rev$
- * @ConQAT.Rating YELLOW Hash: C0892369A081298B96E205693A9EFE4B
+ * @ConQAT.Rating GREEN Hash: EE9B9B5B95019D2E5D724DA8DADFBC5B
  */
 public abstract class SimpleToolRunnerBase extends ToolRunnerBase<INamedElement, List<String>> {
 
@@ -69,7 +69,9 @@ public abstract class SimpleToolRunnerBase extends ToolRunnerBase<INamedElement,
 		return new LazyResultBase(new SimpleToolRunningTask());
 	}
 
-	/** A simple tool runner task. */
+	/**
+	 * A simple tool runner task.
+	 */
 	private class SimpleToolRunningTask extends ToolRunningTask {
 		/** Constructor. */
 		SimpleToolRunningTask() {
@@ -85,7 +87,9 @@ public abstract class SimpleToolRunnerBase extends ToolRunnerBase<INamedElement,
 		}
 	}
 
-	/** The simple result builder returning the console output. */
+	/**
+	 * The simple result builder returning the console output.
+	 */
 	private class SimpleResultBuilder implements IResultBuilder<List<String>> {
 		/** {@inheritDoc} */
 		@Override

org.fortiss.af3.tools/trunk/src/org/fortiss/af3/tools/base/ToolRunnerBase.java

@@ -55,10 +55,12 @@ import org.fortiss.tooling.kernel.model.INamedElement;
  * @author ratiu
  * @author $Author: hoelzl $
  * @version $Rev: 18709 $
- * @ConQAT.Rating YELLOW Hash: 221C995B9EF75829FF1BD2FCB20A1574
+ * @ConQAT.Rating RED Hash: 808FF0487ECD7D3ED822E61D6BF846F5
  */
 public abstract class ToolRunnerBase<S extends INamedElement, T> implements IToolRunner<T> {
 
+	// TODO(AW) review: this flag seems to be overwhelmingly used, is there maybe a cleaner
+	// solution?
 	/** Whether the tool should be debugged. */
 	public static boolean DEBUG = false;
 
@@ -74,13 +76,20 @@ public abstract class ToolRunnerBase<S extends INamedElement, T> implements IToo
 	/** Returns the shell command that runs the tool. */
 	protected abstract String getToolCommand();
 
-	/** Generates the physical file. */
+	/**
+	 * Generates the physical file.
+	 */
 	protected File generatePhysicalFile(ITextGenerator<S> textGen, String fileExtension)
 			throws IOException, UnknownLanguageFragmentException {
 
 		IPath location = plugin.getStateLocation();
 		Random randomGenerator = new Random();
 
+		// TODO(AW) review: Shall this regex remove only the trailing non-alphanumeric character
+		// or all non-alphanumeric characters? Isn't it possible, that ambiguities
+		// occur, when information-bearing characters are just skipped? I mean, are
+		// random numbers really safe? I don't know the complete context, so I may be
+		// Completely wrong ;)
 		String patchedPhysicalFileName = logicalFile.getName().replaceAll("[^A-Za-z0-9]", "");
 		File physicalFile =
 				new File(location.toFile(), patchedPhysicalFileName + randomGenerator.nextLong() +
@@ -126,7 +135,9 @@ public abstract class ToolRunnerBase<S extends INamedElement, T> implements IToo
 		return null;
 	}
 
-	/** Reads the input stream in a thread. */
+	/**
+	 * Reads the input stream in a thread.
+	 */
 	protected class StreamReader extends Thread {
 
 		/** The input stream. */
@@ -187,7 +198,9 @@ public abstract class ToolRunnerBase<S extends INamedElement, T> implements IToo
 		}
 	}
 
-	/** A tool running task. */
+	/**
+	 * A tool running task.
+	 */
 	protected class ToolRunningTask implements Callable<T> {
 
 		/** The file that contains the textual tool code. */
@@ -212,7 +225,8 @@ public abstract class ToolRunnerBase<S extends INamedElement, T> implements IToo
 		public T call() throws Exception {
 			info(plugin,
 					"Running " + getToolCommand() + " " + Arrays.toString(cmdArray) + " on file " +
-							physicalFile.getName() + " in directory " + physicalFile.getParent());
+							physicalFile.getName() + " in directory " + physicalFile.getParent() +
+							".");
 
 			long initialTimeStamp = System.currentTimeMillis();
 
@@ -233,7 +247,7 @@ public abstract class ToolRunnerBase<S extends INamedElement, T> implements IToo
 			String duration =
 					new DecimalFormat("###.###")
 							.format((System.currentTimeMillis() - initialTimeStamp) / 1000.0);
-			info(plugin, "Run in " + duration + "s");
+			info(plugin, "Run in " + duration + "s.");
 
 			return doProcessRunResults(errorStreamReader, inputStreamReader);
 		}
@@ -241,6 +255,7 @@ public abstract class ToolRunnerBase<S extends INamedElement, T> implements IToo
 		/** Processes the results of the tool-run. */
 		protected T doProcessRunResults(StreamReader errorStreamReader,
 				StreamReader inputStreamReader) throws IOException, Exception {
+
 			String errorMessage = errorStreamReader.getReadString();
 			if(errorMessage.length() > 0) {
 				throw new Exception(getToolCommand() + " running errors\n" + errorMessage);
@@ -250,7 +265,7 @@ public abstract class ToolRunnerBase<S extends INamedElement, T> implements IToo
 				error(plugin,
 						"Error on running " + getToolCommand() + " on file: " +
 								physicalFile.getAbsolutePath() +
-								" The file was not deleted and can be debugged! ");
+								" The file was not deleted and can be debugged!");
 				throw new Exception("No result from running " + getToolCommand() + " :-( !!");
 			}
 
@@ -262,7 +277,9 @@ public abstract class ToolRunnerBase<S extends INamedElement, T> implements IToo
 		}
 	}
 
-	/** Base class for a lazy result. */
+	/**
+	 * Base class for a lazy result.
+	 */
 	protected class LazyResultBase implements ILazyResult<T> {
 
 		/** The future result. */

org.fortiss.af3.tools/trunk/src/org/fortiss/af3/tools/gcc/BuiltProgramRunner.java

@@ -32,7 +32,7 @@ import org.fortiss.af3.tools.base.SimpleToolRunnerBase;
  * @author hoelzl
  * @author $Author$
  * @version $Rev$
- * @ConQAT.Rating YELLOW Hash: 3F752FB638ABC102A41713AED5D08583
+ * @ConQAT.Rating RED Hash: F58DDBC26B4ACF4ACF67047916E13713
  */
 public class BuiltProgramRunner extends SimpleToolRunnerBase {
 
@@ -77,6 +77,9 @@ public class BuiltProgramRunner extends SimpleToolRunnerBase {
 		try {
 			return readFile(physicalFile);
 		} catch(IOException e) {
+			// TODO(AW) review: may it be useful to provide an error-log entry about
+			// this incident here? Please ignore this comment, if this exception is just here
+			// for completeness.
 			e.printStackTrace();
 		}
 		return null;

org.fortiss.af3.tools/trunk/src/org/fortiss/af3/tools/gcc/ConfigureRunner.java

@@ -32,7 +32,7 @@ import org.fortiss.af3.tools.base.SimpleToolRunnerBase;
  * @author hoelzl
  * @author $Author$
  * @version $Rev$
- * @ConQAT.Rating YELLOW Hash: F86597C6536CAF9D1253DFD136C68EDB
+ * @ConQAT.Rating RED Hash: B908F969DDF4B1CFEF7B93B66C2D3854
  */
 public class ConfigureRunner extends SimpleToolRunnerBase {
 
@@ -72,6 +72,9 @@ public class ConfigureRunner extends SimpleToolRunnerBase {
 		try {
 			return readFile(physicalFile);
 		} catch(IOException e) {
+			// TODO(AW) review: may it be useful to provide an error-log entry about
+			// this incident here? Please ignore this comment, if this exception is just here
+			// for completeness.
 			e.printStackTrace();
 		}
 		return null;

org.fortiss.af3.tools/trunk/src/org/fortiss/af3/tools/gcc/GCCRunner.java

@@ -25,7 +25,7 @@ import java.io.File;
  * @author hoelzl
  * @author $Author$
  * @version $Rev$
- * @ConQAT.Rating YELLOW Hash: 23DF64B14ECC37F23B3F50BCCBD6043F
+ * @ConQAT.Rating RED Hash: 80CD5F1A5E715F57A5D8F4EB63CD9744
  */
 public class GCCRunner extends GCCToolChainRunnerBase {
 
@@ -54,6 +54,7 @@ public class GCCRunner extends GCCToolChainRunnerBase {
 	/** {@inheritDoc} */
 	@Override
 	protected String getVersionInfo(String helpMessage) {
+		// TODO(AW) review: please see 'ShellRunner'.
 		int startOfVersioningInfo = helpMessage.indexOf("gcc version ");
 		return helpMessage.substring(startOfVersioningInfo + 12, startOfVersioningInfo + 17);
 	}

org.fortiss.af3.tools/trunk/src/org/fortiss/af3/tools/gcc/GCCToolChainRunnerBase.java

@@ -28,7 +28,7 @@ import org.fortiss.af3.tools.base.SimpleToolRunnerBase;
  * @author hoelzl
  * @author $Author$
  * @version $Rev$
- * @ConQAT.Rating YELLOW Hash: E4120CC30D3AA710E29043DE07DC4C6F
+ * @ConQAT.Rating GREEN Hash: 863C0D40D824B26F915B1DEA3F30C7B1
  */
 public abstract class GCCToolChainRunnerBase extends SimpleToolRunnerBase {
 
@@ -57,7 +57,7 @@ public abstract class GCCToolChainRunnerBase extends SimpleToolRunnerBase {
 		return null;
 	}
 
-	/** Flag for using normal or error output for version info. The default uses the normal output. */
+	/** Flag for using normal or error output for version info. The default uses normal output. */
 	protected boolean useErrorOutputForVersion() {
 		return false;
 	}

org.fortiss.af3.tools/trunk/src/org/fortiss/af3/tools/gcc/MakeRunner.java

@@ -22,14 +22,15 @@ import static org.eclipse.core.runtime.Assert.isTrue;
 import java.io.File;
 
 /**
- * Tool runner class for running Make.
+ * Tool runner class for running the "make"-tool.
  * 
  * @author hoelzl
  * @author $Author$
  * @version $Rev$
- * @ConQAT.Rating YELLOW Hash: 458B9D9706DEF2FD95615D1AA21E23A5
+ * @ConQAT.Rating RED Hash: AA2EBC818460EE1D5A931C87800D13CB
  */
 public class MakeRunner extends GCCToolChainRunnerBase {
+
 	/** Constructor. */
 	public MakeRunner(File makeFile) {
 		super(makeFile);
@@ -56,6 +57,7 @@ public class MakeRunner extends GCCToolChainRunnerBase {
 	/** {@inheritDoc} */
 	@Override
 	protected String getVersionInfo(String helpMessage) {
+		// TODO(AW) review: please see 'ShellRunner'.
 		int startOfVersioningInfo = helpMessage.indexOf("GNU Make ") + 9;
 		return helpMessage.substring(startOfVersioningInfo, startOfVersioningInfo + 4);
 	}

org.fortiss.af3.tools/trunk/src/org/fortiss/af3/tools/gcc/ShellRunner.java

@@ -26,7 +26,7 @@ import java.io.File;
  * @author hoelzl
  * @author $Author$
  * @version $Rev$
- * @ConQAT.Rating YELLOW Hash: E9CF3DEB3772DC1BBED28907BD6993CF
+ * @ConQAT.Rating RED Hash: AC236F369217514A802B5FFF69CBC120
  */
 public class ShellRunner extends GCCToolChainRunnerBase {
 
@@ -62,6 +62,11 @@ public class ShellRunner extends GCCToolChainRunnerBase {
 	@Override
 	protected String getVersionInfo(String helpMessage) {
 		int startOfVersioningInfo = helpMessage.indexOf("version ") + 8;
+		// TODO(AW) review: this type of parsing can be dangerous, if the version string is
+		// extended in the future. So it may be possible that "1.42.9", for example, increments to
+		// "1.42.10" which would produce an error here. The magic version string length of "6"
+		// seems a bit arbitrary to me ;) ... maybe there is currently no better solution, then
+		// please ignore this comment.
 		return helpMessage.substring(startOfVersioningInfo, startOfVersioningInfo + 6);
 	}
 }

org.fortiss.af3.tools/trunk/src/org/fortiss/af3/tools/gcc/package.html

+<!--  
+  $Id$
+  @version $Rev$
+  @ConQAT.Rating RED Hash: E522E0A83F619710EDF84EEDFB309EC4
+-->
+<body>
+review: this file was missing.
+</body>

org.fortiss.af3.tools/trunk/test-src/test/org/fortiss/af3/tools/gcc/SmokeTest.java

@@ -31,9 +31,10 @@ import org.junit.Test;
  * @author hoelzl
  * @author $Author$
  * @version $Rev$
- * @ConQAT.Rating YELLOW Hash: 7AC09041F21CAFF6E75535FECA1E4991
+ * @ConQAT.Rating GREEN Hash: 60C56F006F6907BD223656BD7F46010E
  */
 public class SmokeTest {
+
 	/** Test GCC availability. */
 	@Test
 	public void gccAvailabilityTest() {

org.fortiss.af3.tools/trunk/test-src/test/org/fortiss/af3/tools/gcc/package.html

 <!--  
   $Id$
   @version $Rev$
-  @ConQAT.Rating YELLOW Hash: E12C4CB49AD4C2ECFBFAE7D9FF4A0F89
+  @ConQAT.Rating GREEN Hash: 2B94854C5F2CC332AC68FC9ABD7152A9
 -->
 <body>
-Base package for tests of GCC-based tool chain in AF3.
-These tests include typical GCC-like call-chain consisting of
+Base package for tests of the GCC-based tool chain in AF3.
+These tests include a typical GCC-like call-chain consisting of
 <OL>
 <LI>"./configure" script</LI>
-<LI>"make all" script (which calls gcc compiler and ld linker)</LI>
+<LI>"make all" script (which calls the gcc compiler and the ld linker)</LI>
 <LI>executable (i.e. the result of the build)</LI>
 <OL> 
 </body>