On Mon, Jul 13, 2009 at 08:48:00PM +0200, Diederik van Lierop wrote:

On 07/13/2009 05:26 AM, Peter Moulder wrote:

...

I've changed libnr/nr-macros.h to prefer our version of CLAMP over glib's, and I've changed both sp-star.cpp and star-context.cpp to call NR_CLAMP explicitly, but I haven't been able to test here.

Diederik, can you svn up and test please?

Thanks for looking into this, this indeed fixed the issue!

Can we now safely replace all occurrences of CLAMP with NR_CLAMP, with the v0.47 release being this close? Or should we be safe and wait until after the release?

Some information relevant to the decision:

- There are apparently 103 lines containing the word CLAMP in inkscape, including just one definition. ("Word" as in grep -w, so excluding CLAMP_D_TO_U8, NR_CLAMP, etc.)

This is a bit much to analyse each one, though of course one could analyse just a few and change just a few.

- Both versions are simple macros that evaluate their arguments multiple times.

- NR_CLAMP requires that that the bounds be ordered (a <= b), and that neither is NaN. So when deciding whether or not to change a particular call of CLAMP to NR_CLAMP, we might take into consideration whether a,b are constants or not, or how certain we are that a <= b (and that neither is NaN).

- glib CLAMP(val, a, b) returns NaN if val is NaN (regardless of the values of a,b); whereas NR_CLAMP returns a for this case[*1].

It seems unlikely that a caller within Inkscape would prefer NaN over a, though it's hard to say.

If we don't do it before releasing 0.47, then I'd support doing it after 0.47, even if it were a blind search-and-replace; at least in the common case that a and b are constants and a < b.

I see the trade-offs as:

- For each individual call, it is likely that changing to NR_CLAMP will either make no functional change (e.g. NaN isn't possible, e.g. it's an int) or will be an improvement.

- I would go so far as to say that it's unlikely that any caller will be worse off being changed to NR_CLAMP, and I'd guess that at least one caller will benefit in at least some circumstance (even if rare).

- Although I don't follow inkscape bug reports very closely, I would guess that in practice it doesn't often cause serious bugs: because I would guess that any serious bug would have gotten attention and been fixed. (One might regard the current case as evidence either for or against this claim.)

- The concern for holding off is of course that we might introduce a bug: maybe there's some code actually ends up behaving better if CLAMP returns NaN than _a, or maybe the slight different macro expansion properties cause a problem, or maybe we make a mistake when changing CLAMP to NR_CLAMP (and we might not notice the mistake if there are 102 callers), or maybe there's some problem I just haven't considered.

There's some evidence that any existing bugs from use of CLAMP aren't too serious or common, whereas possibly an introduced bug will be serious (e.g. perhaps making some code not work at all rather than fail only in the rare occasion that NaN is encountered).

There isn't much testing time remaining, which reduces the probability that any such serious bug would be found between committing the change and releasing 0.47.

One of the changes I made to libnr/nr-macros.h was to make it override any existing definition of CLAMP from glib. Subject to anyone else's comments in the thread, I may revert that aspect of the change, just in case, though I think it should be re-instated after 0.47.

pjrm.

[*1] I have seen one or two inkscape bug reports whose symptoms indicate that some compiler used under Windows doesn't have correct NaN comparison behaviour. I don't know whether such a bug still exists in current compiler versions or not, or how common such a bug is.