I have a few comments thrown in below..
On Wed, Jun 03, 2009 at 07:27:46PM +0200, micges wrote:
> - if (readRetval > INTERP_MIN_ERROR || readRetval == 3
> /* INTERP_ENDFILE
> -
> */ ||
> - readRetval == 1 /* INTERP_EXIT */ ||
> - readRetval == 2 /* INTERP_ENDFILE,
> - INTERP_EXECUTE_FINISH */ ) {
> - /* emcTaskPlanRead retval != INTERP_OK
> - Signal to the rest of the system that that the
> interp
> + if (readRetval != INTERP_OK) {
> + /* Signal to the rest of the system that that the
> interp
> is now in a paused state. */
> - /*! \todo FIXME The above test *should* be reduced
> to:
> - readRetVal != INTERP_OK
> - (N.B. Watch for negative error codes.) */
> + /* (N.B. Watch for negative error codes.) */
Please help me understand why the totally different test is actually
equivalent.
If they really are equivalent, please go back in time and make the
person who wrote the comment actually CHANGE THE CODE instead of writing
the comment. (OK, that part may be too difficult)
> if (execRetval == -1 /* INTERP_ERROR */ ||
> - execRetval > INTERP_MIN_ERROR || execRetval ==
> 1 /* INTERP_EXIT
> -
> */ ) {
> + execRetval > INTERP_MIN_ERROR ||
> + execRetval == INTERP_EXIT) {
It seems that as long as you're in here, you should get rid of the
misleading /* INTERP_ERROR */ test -- as far as I can tell, INTERP_ERROR
is 5, not -1.
> @@ -1012,17 +961,18 @@
> }
>
> if (emcStatus->task.readLine < programStartLine) {
> -
> - //update the position with our current
> position, as the other positions are only skipped through
> -
> CANON_UPDATE_END_POINT(emcStatus->motion.traj.actualPosition.tran.x,
> -
> emcStatus->motion.traj.actualPosition.tran.y,
> -
> emcStatus->motion.traj.actualPosition.tran.z,
> -
> emcStatus->motion.traj.actualPosition.a,
> -
> emcStatus->motion.traj.actualPosition.b,
> -
> emcStatus->motion.traj.actualPosition.c,
> -
> emcStatus->motion.traj.actualPosition.u,
> -
> emcStatus->motion.traj.actualPosition.v,
> -
> emcStatus->motion.traj.actualPosition.w);
> + //update the position with our current
> position,
> + //as the other positions are only skipped
> through
> + CANON_UPDATE_END_POINT(
> + emcStatus->motion.traj.actualPosition.tran.x,
> + emcStatus->motion.traj.actualPosition.tran.y,
> + emcStatus->motion.traj.actualPosition.tran.z,
> + emcStatus->motion.traj.actualPosition.a,
> + emcStatus->motion.traj.actualPosition.b,
> + emcStatus->motion.traj.actualPosition.c,
> + emcStatus->motion.traj.actualPosition.u,
> + emcStatus->motion.traj.actualPosition.v,
> + emcStatus->motion.traj.actualPosition.w);
Personally I don't find much value in whitespace-only changes like this.
We'll never have totally consistent whitespace in emc, and even if we
did it would still dissatisfy at least some of the developers in some
cases. Better to leave the lines alone to make the output of "cvs
annotate" a little more useful.
> if (execRetval == 2 /* INTERP_ENDFILE */ ) {
Why not change this one as long as you're in here? (of course,
INTERP_ENDFILE is not 2 in my copy of emc!)
Jeff
------------------------------------------------------------------------------
OpenSolaris 2009.06 is a cutting edge operating system for enterprises
looking to deploy the next generation of Solaris that includes the latest
innovations from Sun and the OpenSource community. Download a copy and
enjoy capabilities such as Networking, Storage and Virtualization.
Go to: http://p.sf.net/sfu/opensolaris-get
_______________________________________________
Emc-developers mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/emc-developers
