fx logical operations should short circuit

Post any defects you find in the released or beta versions of the ImageMagick software here. Include the ImageMagick version, OS, and any command-line required to reproduce the problem. Got a patch for a bug? Post it here.
Locked
zweibieren
Posts: 4
Joined: 2015-08-01T14:33:34-07:00
Authentication code: 1151

fx logical operations should short circuit

Post by zweibieren »

In fx, the logical operations && and || always evaluate both arguments. This is wrong. It will also greatly expand the time for the script I have just written. The change to the source code is minimal:

old:

Code: Select all

        case LogicalAndOperator:
        {
          gamma=FxEvaluateSubexpression(fx_info,channel,x,y,++p,beta,exception);
          *beta=(alpha > 0.0) && (gamma > 0.0) ? 1.0 : 0.0;
          return(*beta);
        }
        case LogicalOrOperator:
        {
          gamma=FxEvaluateSubexpression(fx_info,channel,x,y,++p,beta,exception);
          *beta=(alpha > 0.0) || (gamma > 0.0) ? 1.0 : 0.0;
          return(*beta);
        }
new (untested)

Code: Select all

        case LogicalAndOperator:
        {
          if (alpha <= 0.0) 
          	return (*beta=0.0);  // alpha is false: the AND is false
          gamma=FxEvaluateSubexpression(fx_info,channel,x,y,++p,beta,exception);
          *beta= (gamma > 0.0) ? 1.0 : 0.0;
          return(*beta);
        }
        case LogicalOrOperator:
        {
          if (alpha > 0.0) 
          	return (*beta=1.0);  // alpha is true: the OR is true
          gamma=FxEvaluateSubexpression(fx_info,channel,x,y,++p,beta,exception);
          *beta=(gamma > 0.0) ? 1.0 : 0.0;
          return(*beta);
        }
Some developers may feel that this could possibly break some scripts. Breaking scripts is a "bad thing" and should be avoided. In this case, however, breakage can only occur when expressions have side effects--and none of the FX operations or primitives have side effects. The only effect of the change will be to make some scripts faster.

If the only hindrance to accepting the change is testing, and if I can be assured that the change will be made after the test is done, I will be happy to do the test.

User avatar
dlemstra
Posts: 1625
Joined: 2013-05-04T15:28:54-07:00
Authentication code: 6789
Contact:

Re: fx logical operations should short circuit

Post by dlemstra »

Your change neglects to increase 'p' but I think it will be safe to make that change if you also increment 'p',
.NET + ImageMagick = Magick.NET https://github.com/dlemstra/Magick.NET, @MagickNET, Donate

User avatar
magick
Site Admin
Posts: 11254
Joined: 2003-05-31T11:32:55-07:00

Re: fx logical operations should short circuit

Post by magick »

We can reproduce the problem you posted and have a patch in ImageMagick 6.9.2-0 Beta, available by sometime tomorrow. Thanks.

zweibieren
Posts: 4
Joined: 2015-08-01T14:33:34-07:00
Authentication code: 1151

Re: fx logical operations should short circuit

Post by zweibieren »

Great! Thanks very much.

Sorry about not incrementing p. I did not read the code closely enough.

Locked