Dear Pierangelo, Pierangelo Masarati schrieb:
Chris Dams writes:
My guess is that the test is incorrect on those architectures, but I admit I haven't investigated enough.
I'm not a CLN expert either, but to me it does not seem that the code is incorrect. On the AMD 64 platform it is apparently always okay to apply the constructor cl_I(int) no matter how large the integer argument is. If the compiler is smart enough, it will automatically do what your patch is doing (i.e., throw the if out of the code), so I fail to see benefits from your patch.
OK, my main concern was about removing a couple annoying warnings (BTW: congratulations: they were the only warnings in the whole package, which seems pretty unusual to me, and a good programming practice). However, as far as I understand, although the code being safe, a test and a cast will always occur even if it's never required. It's not a big deal, I concur, but it looks like a misuse of the CLN definitions. My 2c.
thanks for your patch! We had a discussion about it on the other GiNaC mailing list (ginac-devel) and we are going to apply it. It will be part of the forthcoming 1.3.5 release then. @Richy: You wondered why the other ctor for unsigned int doesn't raise such a warning. After looking at the code I now wonder whether the code there is optimal. The argument is compared against 2^(cl_value_len-1) and not 2^(cl_value_len)-1 as one might expect. Maybe this should be changed? Could you comment on this soon, because I'd like to roll the release this evening? Regards, Jens