Jeff Epler napisał(a):
> 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.
>
above code tests readRetval with all values of INTERP_xxx defines
readRetval > INTERP_MIN_ERROR that ecuals 4 or 5
and we have 1, 2 and 3 so there are tested all cases different than
INTERP_OK
>> 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.
>
>
that's correct
>> @@ -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
>
>
there is no such line in source or diff.
I'll examine those changes once again and commit new patch when approved.
regards,
Michael
------------------------------------------------------------------------------
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
