Hello all,

I've got a program that correctly computes the largest factor in the prime decomposition of a positive number:

*****************************************************
import std.math std.stdio;

ulong largestPrimeFactor(ulong n) {
    for(ulong p=2; p<=sqrt(cast(real)n); ) {
        if(n%p==0)
            n/=p;
        else
            p+=1;
    }
    return n;
}

void main() {
    writeln(largestPrimeFactor(4));
}
*****************************************************

However, if I replace the content of the for loop with the ?: operator, the program is not
correct anymore (largestPrimeFactor(4) now returns 3):

*****************************************************
import std.math std.stdio;

ulong largestPrimeFactor(ulong n) {
    for(ulong p=2; p<=sqrt(cast(real)n); ) {
        n%p==0 ? n/=p : p+=1 ;
    }
    return n;
}

void main() {
    writeln(largestPrimeFactor(4));
}
*****************************************************

What am I doing wrong here?
I'm using dmd version 2.071.1-0 on ubuntu.

On Tuesday, 26 July 2016 at 13:09:28 UTC, Richard wrote:
> Hello all,

try using some parentheses:
(n%p==0) ? (n/=p) : (p+=1) ;

On Tuesday, 26 July 2016 at 13:09:28 UTC, Richard wrote:
>         n%p==0 ? n/=p : p+=1 ;


Try

(n%p==0) ? n/=p : p+=1 ;


I'm pretty sure the order of operations (precedence rules) puts ?: above ==.

On Tuesday, 26 July 2016 at 13:17:27 UTC, Adam D. Ruppe wrote:
> (n%p==0) ? n/=p : p+=1 ;


And actually, using ?: with /= and += is kinda bizarre.

I think you are better off leaving this as if/else since the point of this is the assignment rather than the return value.

On 07/26/2016 03:09 PM, Richard wrote:
>         if(n%p==0)
>             n/=p;
>         else
>             p+=1;
[...]
> However, if I replace the content of the for loop with the ?: operator,
> the program is not
> correct anymore (largestPrimeFactor(4) now returns 3):
[...]
>         n%p==0 ? n/=p : p+=1 ;
[...]
> What am I doing wrong here?

Operator precedence is different from what you think. `a ? b : c = d` means `(a ? b : c) = d`. But you want `a ? b : (c = d)`. So you need parentheses around `p+=1`.

Or just go with `if` and `else`. It's clearer anyway.

On Tuesday, 26 July 2016 at 13:19:54 UTC, ag0aep6g wrote:
> Operator precedence is different from what you think. `a ? b : c = d` means `(a ? b : c) = d`. But you want `a ? b : (c = d)`. So you need parentheses around `p+=1`.
>
> Or just go with `if` and `else`. It's clearer anyway.

From http://wiki.dlang.org/Operator_precedence

Priority(15 is highest)
3                      Conditional operator                  ?: 	
2                      Assignment operators    = -= += <<= >>= >>>= = *= %= ^= ^^= ~=

I was actually close to not needing parentheses :). But I see that your suggestion to stick with if else in this case is the sensible thing to do, especially since ?: seems to lead to more errors. Thanks for the answers.

On Tuesday, July 26, 2016 13:41:39 Richard via Digitalmars-d-learn wrote:
> On Tuesday, 26 July 2016 at 13:19:54 UTC, ag0aep6g wrote:
> > Operator precedence is different from what you think. `a ? b :
> > c = d` means `(a ? b : c) = d`. But you want `a ? b : (c = d)`.
> > So you need parentheses around `p+=1`.
> >
> > Or just go with `if` and `else`. It's clearer anyway.
>
>  From http://wiki.dlang.org/Operator_precedence
>
> Priority(15 is highest)
> 3                      Conditional operator                  ?:
> 2                      Assignment operators    = -= += <<= >>=
>
>  >>>= = *= %= ^= ^^= ~=
>
> I was actually close to not needing parentheses :). But I see that your suggestion to stick with if else in this case is the sensible thing to do, especially since ?: seems to lead to more errors. Thanks for the answers.

The ternary operator is invaluable for stuff like initializing a constant based on boolean condition, and it can be useful in code in general, but I think that most everyone would agree that you don't want to be doing mutating operations inside of a ternary operator and that if/else statements are far better suited to that - particularly since the ternary operator results in a value, which is not at all what you're looking to do here.

That being said, it surprises me how often folks get confused by the precedence of the ternary operator and start throwing parens around it. The only reason that you'd need to here is becasue of the assignment operations, which really shouldn't be used in a ternary expression anyway (except when using the result of the whole expression as the value to assign).

- Jonathan M Davis

On 07/26/2016 06:41 AM, Richard wrote:

> From http://wiki.dlang.org/Operator_precedence

In case it's useful to others, I explain that table a little bit here (associativity, unordered operators, and the precedence of =>):

  http://ddili.org/ders/d.en/operator_precedence.html

Ali