On Thu, Jul 9, 2015 at 7:46 AM, Nick Wellnhofer <[email protected]> wrote:
> On 09/07/2015 04:53, Marvin Humphrey wrote:
>>
>> In retrospect, I regret the Num inheritance hierarchy. I wish that
>> instead, we had only Boolean, Integer, and Float, matching the types
>> available in JSON. That wouldn't solve all our problems, it would just
>> give us a different set of problems -- but I feel like that set of problems
>> is more appropriate for Clownfish to have.
>
> I agree. Here is a branch that implements most of your ideas.
>
> https://github.com/nwellnhof/lucy-clownfish/commits/num_hierarchy
> https://github.com/nwellnhof/lucy/commits/num_hierarchy
Wow. I thought we had disagreed about this in the past, so I was prepping
for some constructive debate. ;) But now, not only have we reached agreement
on all important issues, you've coded the whole thing up!
I've reviewed the branches and the changesets all look sound. +1 to merge!
The one change I'm a little leery of is this:
int64_t
Float64_To_I64_IMP(Float64 *self) {
+ if (self->value < -POW_2_63 || self->value >= POW_2_63) {
+ THROW(ERR, "Float64 out of range: %f64", self->value);
+ }
return (int64_t)self->value;
}
I understand the rationale, though, and it's better to add the exception while
we're focused on this code than it is to leave it be. Perhaps we can convey
the error via a MAYBE return type in the future...
I also checked the conversion code in Lucy's SortFieldWriter, here:
https://github.com/nwellnhof/lucy/commit/8144f5c7f3806873b58a9f1e751243f2c294eb13#diff-6c25c769e6f97d81aa1663f8781965c2L276
The lossy cast is a decent approach. It would also have been reasonable to
throw an exception on overflow, but the chances of truncation-on-overflow
causing a serious problem seem remote.
> Execpt for:
>
>> * Integer, Float, and Boolean would all live in their own files rather
>> than in Num.cfh/Num.c.
OK, no big deal.
> Boolean is immutable already. Making Integer and Float immutable requires
> some adjustments to Lucy's Inverter similar to the changes for immutable
> Strings.
Looks good!
Marvin Humphrey
