RFC: Totally ordered wrappers for f32 and f64 (and possibly others)

[lifthrasiir]

It is well known that f32 and f64 are not totally orderable by default, and thus does not implement TotalOrd and TotalEq. However it is less known that we rarely need the total ordering in reality, and the standard library has only two cases requiring the total ordering (sorting and TreeMap). #12434 and #12435 are based on this observation, but we can think in the other direction: we can provide totally ordered wrappers for partially ordered types.

The suggested wrappers, TotallyOrderedF32 and TotallyOrderedF64 will be tuple (newtype) structs for f32 and f64. (Yes, I know this name doesn't look good, it's intentional.) They have the following constraints: (TOF for any of two types)

x < y  and x == x and y == y implies TOF(x).cmp(&TOF(y)) == Less
x > y  and x == x and y == y implies TOF(x).cmp(&TOF(y)) == Greater
x == y and x == x and y == y implies TOF(x).cmp(&TOF(y)) == Equal
           x != y or  y != y implies TOF(x).cmp(&TOF(y)) is arbitrary (but a function of x/y)

The actual implementation would be similar to IEEE 754-2008 totalOrder except that TOF(-0).cmp(&TOF(+0)) == Equal, otherwise we would break the constraints as -0 == +0. Combined with Deref (#7141, which would have to be implemented eventually) I no longer see the separation of Ord/Eq and TotalOrd/Eq is necessary. Usability-wise, the compiler can suggest TotallyOrderedF{32,64} on the failure to expand #[deriving(TotalOrd)] (and others) for f32 and f64.

This RFC only concerns about f32 and f64 since they are commonly encountered non-totally-ordered types, but if we want to pursue this further, we can alternatively provide a TotalOrder generic wrapper that massages Ord implementors to aforementioned constraints.

[thestinger]

However it is less known that we rarely need the total ordering in reality, and the standard library has only two cases requiring the total ordering (sorting and TreeMap).

Is there any generic code built on Eq and Ord that's correct in the presence of floating point types? If the answer is no, then the traits are not useful. I think the answer is no, because even min, max and clamp require specialized versions for floating point. The HashMap implementation is also incorrect as it really requires TotalEq (#5283).

The #12435 pull request is intended to remove a set of useless traits from the standard library, and make the usable ones into the default. The standard library is simply too complex, and needs to limit the usage of traits to cases where they're helpful for writing generic code. It's silly to have a set of traits for overloading operators not usable by generic code and then another set of traits with real guarantees but no operating overloading. Floating point types do have a total ordering available, so providing a terrible API for the sake of floating point seems silly.

I think it's important to keep in mind that if traits only have type signatures but no laws, then the advantage of explicitly declaring type bounds instead of inferring the requirements from the implementation is pretty much gone. The traits aren't really documenting anything about the API if they don't provide either a strict weak order or total order.

[pnkfelix]

"It's silly to have a set of traits for overloading operators not usable by generic code and then another set of traits with real guarantees but no operating overloading"

If each trait that provided the guarantees extended the trait that defined the overloaded operator, then I'm not sure that's totally silly. (I mean, it would be a little silly, in that the use of a trait to define the operator is solely an artifact of how Rust does operator-overloading. But not completely silly.)

E.g. I'm thinking something like this:

#[lang="ord"]
/// This trait is solely to provide access to {<, >, <=, >=} sugared operators,
///  but there are no guarantees about their semantics.  It has an intentionally ugly
/// name to discourage use as an accidental trait-bound on type parameters.
pub trait OrdOps {
    fn lt(&self, other: &Self) -> bool;
    #[inline]
    fn le(&self, other: &Self) -> bool { !other.lt(self) }
    #[inline]
    fn gt(&self, other: &Self) -> bool {  other.lt(self) }
    #[inline]
    fn ge(&self, other: &Self) -> bool { !self.lt(other) }

    // FIXME (#12068): Add min/max/clamp default methods
}

/// This trait provides concrete guarantees about ordering.
/// Note that f32/f64 does not impl this trait, but most other numerics do.
trait Ord : OrdOps {  }

Note that then #[deriving(Ord)] would need to be changed to inject definitions for both OrdOps and Ord.

Having said that, I do not do enough floating-point work myself to push hard to support {<, >, <=, >=} as built-in operators for f32/f64. This was just something I thought of this morning that I thought might resolve this issue. (Its possible that someone had already suggested this elsewhere.)

[thestinger]

@pnkfelix: IEEE754 defines a total ordering and we are going to implement it, so leaving out an implementation of the total ordering trait for floating point isn't a good option. This means the traits can't have any inheritance relationship if the operators are overloaded with the weak ordering for floating point types. So, there's a trait for generic code and a trait for operator overloading with the overloaded operators not usable in generic code.

[pnkfelix]

@thestinger my assumption is/was that people working with floating point might prefer to use the operator notation for the non-total-ordering. But I admit this would make it hard/impossible to mix generic-code that uses the Ord bound with structures that are employing floats (since the whole point of my suggestion was to make f32/f64 implement OrdOps and not Ord).

[pnkfelix]

@thestinger Oh wait, maybe I misunderstood part of your response.

You said: "So, there's a trait for generic code and a trait for operator overloading with the overloaded operators not usable in generic code."

Does this mean you can have more than one trait providing access to the same overloaded operators? (And presumably choose between them via appropriate use of use). If so, (1.) I did not know that, and (2.) Rust gets cooler every day.

(Update: rust may get cooler every day, but the above hypothesis is flawed, because you cannot currently have more than one trait with the #[lang="ord"] attribute. Not sure at the moment if that idea is forever impossible or if we might be able to hack it in post 1.0. It does not matter terribly for now.)

[thestinger]

I mean that as long as the floating point numbers use the non-total-ordering for the comparison operators, Rust won't have usable comparison operator overloading in generic code. Even the min and max functions are not correct for floating point types, and need to be switched to TotalOrd. The HashMap container needs to be switched to TotalEq and ordered maps/sets and sorting already use TotalOrd. I think it's a real disaster at the moment.

[pnkfelix]

@thestinger So just to be clear, when you said "So, there's a trait for generic code and a trait for operator overloading with the overloaded operators not usable in generic code.", this second trait, not usable in generic code (which presumably would be solely for f32/f64): you did not mean that it would provide access to infix {<, >, <=, >=} for f32/f64, right? Just that it would provide access to methods with names like float_lt, float_gt, etc (or something along those lines) that use the hardware's floating point comparison operations?

(I took your use of the word "overloaded operators" to mean "infix sugar", but it seems you could not have meant that.)

[thestinger]

No, I think you're misunderstanding what I'm saying. I am only saying that today, TotalOrd is for generic code and Ord is used only for the operator overloads. The situation today is that Eq and Ord are nearly useless traits, simply bloating the API documentation and causing confusion. These traits are unspecified, without any guarantees, so generic code can't make use of them. Floating point types would cause nearly any generic code to be incorrect since as generic code it's not going to include the necessary handling of NaN like fmin, fmax, a float-specific clamp, etc.

The TotalEq and TotalOrd traits are the ones usable by generic code, but do not provide operator overloads. Operators have to be tied to be a specific trait (in this case, Eq and Ord) or there will be coherency issues. It's not possible for these to inherit from Eq and Ord for the operator overloads because floating point types will not be implementing the same operations for both. Therefore, generic code has not access to operator overloads at the moment. There aren't convenience methods either (names taken by Ord), so it has to call cmp and compare against the Ordering variants.

[pnkfelix]

@thestinger Okay. You may not believe this, but I have understood the issues you have describing about Ord and Eq not providing any built-in guarantees that all-implementors adhere to, and I agree with the philosophy you have been espousing (namely that Ord/Eq are useless as trait bounds unless they do have some semantic laws associated with them).

I was just musing about a way to try to keep the operator-sugar available for floating-point hackers. I acknowledge that the separation I was proposing has similar problems to that of the existing Ord/TotalOrd separation; I was mostly just hoping that it would be less confusing (since the distinct roles of "provide operator sugar" and "provide semantic guarantees" would be better distinguished).

But I am fine with forcing people who want the hardware f32/f64 operations to have to use different methods without infix sugar.

[thestinger]

The previously existing proposal is #12435. The Eq and Ord traits provide the strong guarantees and operator overloads. There will be no need for partial or total ordering traits as it's simply not possible to provide this with a sane trait hierarchy. Floating point types will implement Eq and Ord with the IEEE754 totalOrder predicate, so deriving and generic code will work correctly.

The faster hardware floating point ordering can be provided by the Float trait as fcmp, fmin, fmax, fclamp, feq, fne, flt, [...]. It doesn't need to introduce complexity into the trait hierarchy.

• generic code should able to use operator overloads
• generic hash tables, ordered maps/sets, min, max, clamp and sorting require more than the hardware floating point comparisons can provide - some of these can be done with it, but doing it correctly requires deviating from a fast implementation (checks for NaN, inability to use three-way comparison functions like memcmp)
• floating point types have both the weak partial ordering and a totalOrder predicate defined in IEEE754

From a purely performance point of view, LLVM needs you to mark floating point operations with the permitted error and relax the restrictions on floating point transformations to allow generating fast code anyway.

[thestinger]

(nevermind the above, it was written before you posted the latest comment)

[glaebhoerl]

I believe the situation can be summarized as, out of the following:

1. Allow use of comparison operator overloads in generic code (rather than methods),
2. Have the comparison operators on f32 and f64 do what people are accustomed to, and what hardware implements (rather than the total ordering defined separately by IEEE754),
3. Allow use of generic code with f32 and f64

Choose any two. @thestinger chooses 1. + 3., @pnkfelix is proposing 1. + 2.

1. • 2. is possible if we have separate a trait OrdOps for the operator overloads, and a trait Ord: OrdOps which decrees a total order and that if a type impls Ord, its OrdOps impl must also be consistent with that order. f32 and f64 implement OrdOps according to the usual hardware-provided semantics, which do not provide a total order, and do not implement Ord. We then separately have newtypes such as struct TotalF32(f32) and struct TotalF64(f64), which implement both OrdOps and Ord according to the IEEE754 total ordering.

[pnkfelix]

@glaebhoerl to be honest, when I proposed 1. + 2., I had not thought through the consequences w.r.t. e.g. structs that have f32/f64 fields (which would then not be able to do deriving(Ord) under my proposal). ((I do acknowledge that your newtype wrappers would be a way to accommodate those cases.))

I think I'd prefer 1. + 3. over 1. + 2., given our current set of constraints. The only person, I think, who suffers under 1.+3. is the poor floating-point hacker, and while I would love to say "oh haven't they suffered enough?", I think if we choose nice-looking + short names for the methods then we will be okay.

[glaebhoerl]

How much potential is there for people who are not "floating-point hackers" per se, but happen to have stumbled into using f32/f64 for some particular task, to be bitten by this?

Essentially, I think that means, if you're not aware of the intricacies of floating-point math and just (naively) expect the comparison operators to do something reasonable, does the IEEE754 total ordering qualify?

[pnkfelix]

@glaebhoerl do you mean bitten w.r.t. the unexpected performance hit, or bitten w.r.t. the difference in semantics of the results of the operators?

[zkamsler]

For situations that do not involve sorting or containers, the default ieee754 comparison semantics may be preferable, or at least surprising. Having -0 not be equal to 0 is is usually more trouble than its worth unless you really care about which infinity you are going to get when dividing by zero.

[glaebhoerl]

@pnkfelix the latter

[pnkfelix]

I think it has been proposed to put in a lint for people using the infix operators directly on f32/f64, so that you'd have to opt in or be warned about the distinction here.

edit: well, maybe it hasn't been previously proposed.

[glaebhoerl]

But if it's a footgun to use them at all, it seems suboptimal to leave it lying around and loaded.

(Disclaimer: I'm very much not a floating-point hacker, which is why I'm asking how much of a footgun it is. I'm inferring from the idea that it deserves a lint that it's a significant one.)

I had not thought through the consequences w.r.t. e.g. structs that have f32/f64 fields (which would then not be able to do deriving(Ord) under my proposal).

Which is just as well! However, they could still do deriving(OrdOps), which is what they would want, I think?

(In theory, if we had something like GHC's Coercible, it would also allow zero-cost conversions between generic types instantiated with f32 and TotalF32 (resp. f64 and TotalF64), which of course we don't yet.)

[zkamsler]

I just want to remind everyone that there is a reason that the IEEE754 comparison predicates were not defined in terms of the totalOrder predicate. Floating point numbers are not intrinsically totally ordered. One can construct a total ordering over them, but that does not imply that the ordering is appropriate for most uses.

I am not aware of any languages that use the total ordering predicate over the standard comparison predicates. Even those that provide an auxiliary total ordering usually do not fully conform to that defined by IEEE754.

[thestinger]

@zkamsler: They are not intrinsically weakly ordered either. There are two valid orderings defined by the standard, and Rust can and should provide both. The comparison operators are defined by the Eq and Ord traits, so if those traits require a total order then that's what floating point types need to implement.

[thestinger]

@glaebhoerl: Defining the operators with the total ordering predicate is very unlikely to cause any bugs. Nearly all software working with floating point is completely ignoring these kinds of issues anyway, and software written with attention spent on these things has far bigger problems to worry about. It will be far more convenient to be able to sort arrays of floating point numbers and derive Eq and Ord for types containing them. If anything, it will prevent more bugs than it causes.

[pnkfelix]

@glaebhoerl to be honest when I wondered about a lint, I was thinking it would be about the performance pitfall anyway.

[zkamsler]

@thestinger I would argue that it is (although weak is perhaps not the right, word having specific meanings). NaN are not numbers, and as such have no logical order relative to anything else, even themselves.

The thing to realize is that an ordering is not the same thing as a comparison. An ordering is free to over-specify. If one is sorting a vector of floats, it does not matter if -0 always goes before 0 and denormals that represent the same number are ordered by their exponents. And NaNs can placed in any arbitrary location, since there is no natural place that they belong. But sorting a before b does not imply that that a is less than b, merely that a is not greater than b. That is the root of the problem.

I do not suggest that the total ordering should not be exposed, but one should not claim that it should be the default basis for the comparison operators.

[thestinger]

@zkamsler: The total ordering guarantee is what generic code requires. If the comparison operators do not use the total ordering traits, then generic code is unable to use the operator overloads. It's not possible to write generic code with an ordering trait when basic guarantees about ordering do not hold.

but one should not claim that it should be the default basis for the comparison operators.

It's not up to you to say what one should or should not claim. IEEE754 specifies a total ordering, and Rust's generic code needs the total ordering. Traits exist for generic code, so it's going to end up being a requirement for the traits and floating point types might as well implement it so they're usable with generic code and deriving. The traditional ordering will still be available along with functions like min, max, clamp and cmp special-cased to handle NaN properly unlike the generic ones.

I'm open to an alternative solution but no one is offering a viable one. Crippling the comparison traits to the point where they're unusable for the sake of traditional floating point semantics isn't acceptable. Having 2 traits that aren't really ever usable along with 2 more traits that are usable but lack the convenience is a bit ridiculous...

If you think the comparison operators are valuable, then push for including user-defined operators, infix call syntax or built-in operators for this. The regular comparison operators belong to Eq and Ord and they need to provide an ordering usable by generic code.

[thestinger]

Rust already uses Option to represent a lack of information in many places, with None == None evaluating to true. You've already lost the battle if you think a lack of information should be considered as inequality in Rust. The only reason it's going to have NaN at all is to support fast hardware floating point. IEEE754 is not the only approximation of real number arithmetic and may not always be the only kind implemented it hardware. A rustic API for sqrt for a non-IEEE754 real number approximation would return Option or fail due to a violated precondition.

[zkamsler]

I am cognizant of the needs of generic code, at least to avoid pathological cases involving NaNs, but an ordering is not the same as comparison. The expression a < b should be true if and only if a is a number that is less than b. This is no longer true if you base the comparison off of the IEEE754 totalOrder predicate.

Even in generic code, the specific totalOrdering specified in the standard may not be the most appropriate. A float-keyed hashtable should probably not put -0 and 0 in different buckets, for example.

[thestinger]

I am cognizant of the needs of generic code, at least to avoid pathological cases involving NaNs, but an ordering is not the same as comparison. The expression a < b should be true if and only if a is a number that is less than b. This is no longer true if you base the comparison off of the IEEE754 totalOrder predicate.

It will remain true with the totalOrder predicate. It's a different ordering, but it's still a valid ordering. You may not be happy that -0.0 will be considered less than 0.0 with the operators but it's just different, not invalid. Ord needs to provide a sane ordering and anything else can be implemented separately.

Even in generic code, the specific totalOrdering specified in the standard may not be the most appropriate. A float-keyed hashtable should probably not put -0 and 0 in different buckets, for example.

It needs to put them in different buckets if the hash isn't the same. The new floating point hash doesn't hash these to the same value. The non-totalOrder comparisons are also broken for a hash table due to x == x returning false for NaN.

[thestinger]

The comparison operators in Rust come from the Ord trait because they correspond to an ordering. It doesn't make to any sense to claim otherwise in the context of Rust. If you want to change how operator overloads work or introduce user-defined operators, then that's a separate issue.

[glaebhoerl]

I'm open to an alternative solution but no one is offering a viable one. Crippling the comparison traits to the point where they're unusable for the sake of traditional floating point semantics isn't acceptable.

Did you grok the plan in my earlier comment? It would allow non-crippled Eq and Ord traits and use of comparison operators in generic code abstracted over them and the "usual" weak order semantics on floating-point comparisons in non-generic code. The price is that if you want to use f32 and f64 with code abstracted over Eq or Ord, you need to wrap them in a newtype first.

Having 2 traits that aren't really ever usable along with 2 more traits that are usable but lack the convenience is a bit ridiculous...

Only two of the traits would ever be used for abstraction, the other two (EqOps / OrdOps) would only exist to define the mechanism for overloading the operators. Or you could go further and make one trait per operator: just like the situation with Num and its "constituents".

(Note: I'm not necessarily in favor of this scheme (I'm not really sure myself), I'm arguing for it to help give it proper consideration.)

Rust already uses Option to represent a lack of information in many places, with None == None evaluating to true. You've already lost the battle if you think a lack of information should be considered as inequality in Rust.

None in Option doesn't mean lack of information, it is more like an empty container (so not lack of knowledge, but knowledge of lack). You could make another type where it does mean that, and where Eq is defined that way. (Or rather you couldn't, if Eq had the laws we want to give it. But you know what I mean.)

[zkamsler]

It is invalid in the sense that it represents the same number for (almost) all intents and purposes, and it is difficult to suggest that multiple subnormal representations of the same number should not be treated as equal.

as an example:

if a < b {
    c = 1 / (b - a);
}
// or similarly
if x < 0 {
    c = 1 / x;
}

With the standard comparison predicates, c is guaranteed to be valid if a and b are finite. But if you use the total ordering predicate, you could easily get get a division by zero.

[thestinger]

None in Option doesn't mean lack of information, it is more like an empty container. You could make another type where it does mean that, and where Eq is defined that way.

It's idiomatic to use it to represent lack of information - this is often how the standard library uses it.

Did you grok the plan in my [earlier comment][1]? It would allow non-crippled Eq and Ord traits and use of comparison operators in generic code abstracted over them and the "usual" weak order semantics on floating-point comparisons in non-generic code. The price is that if you want to use f32 and f64 with code abstracted over Eq or Ord, you need to wrap them in a newtype first.

This issue is opposed to landing #12435. I'm fine with having two sets of floating point types, but there's no way we're going to have more than the Eq and Ord traits. Any idea involving more comparison traits is an idea that I'm going to shoot down. There's already another issue open about floating point types in a post-#12435 world.

[glaebhoerl]

Sure, if you don't want to continue discussing the situation using logical arguments then I'll go find something else to do. Carry on.

[thestinger]

Sure, if you don't want to continue discussing the situation using logical arguments then I'll go find something else to do. Carry on.

I don't understand why you're saying this. I haven't seen any reasons to keep around 4 comparison traits so I edited #12434 to reflect that there's no consensus on how to fix floating point. There's no reason for it to block changes to the comparison traits because the existing Eq and Ord have proven to be unusable in generic ordered containers, sorting and convenience functions.

[pnkfelix]

@thestinger I'm not willing to shut the discussion down like that, so I'm re-opening the issue.

@glaebhoerl and I were not, IMO, suggesting having 4 comparison traits. We were suggesting decoupling the comparison trait from the overloaded infix operators.

[thestinger]

@pnkfelix: There was already a previous issue open about floating point. Decoupling the comparison overloads is suggesting having 4 comparison traits because they are comparison overloads. It's not different than the current situation of having an overly complex API without the ability for generic code to make use of the overloads.

[glaebhoerl]

@thestinger I wholeheartedly agree that Eq and Ord should have meaningful laws, and that code generic over Eq and Ord should be able to use the comparison operators , knowing that they will respect the laws. The thing is that, as far as I can tell, we don't seem to be disagreeing on the merits, rather we don't seem to be on the same page with regards to the facts of the alternative proposal.

Decoupling the comparison overloads is suggesting having 4 comparison traits because they are comparison overloads.

This is true. (Perhaps even more than 4, if we decide to make one trait per operator (as with Add, Sub, etc.).)

without the ability for generic code to make use of the overloads

This is not true. Eq and Ord would inherit the traits providing the operator overloads, and would add laws stating that e.g. to impl Ord for a type, the comparison operators for the type must also be consistent with the total order. A function that's generic over Ord would then be able to use the operators, because Ord would bring them into scope by way of its supertraits, and could also expect that they behave correctly, because of the laws.

To be very explicit, let me sketch code:

#[lang("eq_ops")]
trait EqOps {
    fn eq(&self, other: &Self) -> bool;
    fn ne etc.
}

#[lang("ord_ops")]
trait OrdOps { fn lt, gt, etc. }

// Laws: 
// `a == a`
// `a == b` <=> `b == a`
// `(a == b) && (b == c)` => `a == c`
// `a != b` <=> `!(a == b)`
trait Eq: EqOps { }

// Laws:
// cmp() must implement a total order
// `a < b` <=> `a.cmp(b) == Less`
// `a <= b` <=> `a.cmp(b) != Greater`
// `a == b` <=> `a.cmp(b) == Equal`
// etc. etc.
trait Ord: Eq, OrdOps {
    fn cmp(&self, other: &Self) -> Ordering;
}

impl EqOps for f32 { /* not an equivalence relation */  }
impl EqOps for f64 { /* not an equivalence relation */ }
impl OrdOps for f32 { /* weak order */ }
impl OrdOps for f64 { /* weak order */ }

struct TotalF32(f32)
struct TotalF64(f64)

impl EqOps for TotalF32 { /* equivalence relation */ }
impl EqOps for TotalF64 { /* equivalence relation */ }
impl OrdOps for TotalF32 { /* total order */ }
impl OrdOps for TotalF64 { /* total order */ }
impl Eq for TotalF32 { }
impl Eq for TotalF64 { }
impl Ord for TotalF32 { fn cmp... }
impl Ord for TotalF64 { fn cmp... }

You're right that this superficially resembles the current situation if we rename Eq => EqOps, Ord => OrdOps, TotalEq => Eq, TotalOrd => Ord. However, unlike the current situation, EqOps and OrdOps would not ever be used for abstraction, they would be required to be consistent with Eq and Ord when they exist, which would inherit them, and code abstracting over Eq and Ord could safely use the operators and assume the laws.

Are you philosophically opposed to having separate traits just to define operator overloads? If so, do you have the same objection against Add, Sub, Mul, BitAnd, BitOr, and so on?

Apart from that (which I don't consider a significant issue), the main tradeoffs are around the ergonomics for floating-point types.

Version A:

• The comparison operators on f32 and f64 use the total order, which may be surprising and undesirable (as detailed by @zkamsler)
• The usual weak order operations are available separately through the Floating trait
• f32 and f64 can be used with code requiring Eq and Ord "out of the box"

Version B:

• The comparison operators on f32 and f64 use the same weak order semantics as every other language
• f32 and f64 can't be used with code requiring Eq and Ord, instead you must use TotalF32/TotalF64 (which are freely convertible)
• structs containing f32/f64 could only derive EqOps and OrdOps, not Eq and Ord. structs containing TotalF32/TotalF64 could derive Eq and Ord as usual.

[thestinger]

I closed the pull request and corresponding RFC. The current design of a separate trait for the operator overloads and total ordering can remain in place. I've opened #12517 to cover renaming the traits to reflect the semantics as they are today (as proposed by @glaebhoerl but using Cmp instead of OrdOps as it's unrelated to ordering) and adding trait inheritance so the operators are available in generic code.

I've edited the description in #5585 to reflect that the total ordering needs to be implemented via wrapper types in order to implement the traits. This is a long-standing issue and fixing it isn't a priority, as long as generic code is updated to stop using the operator overloading traits when it needs stronger guarantees.

I don't think we need more than these 2 issues. There is clearly no perfect solution to this, and I am not interested in trying to drastically change the design anymore. It just needs to be clearly defined with the existing generic code fixed.