At Thu, 2 Aug 2018 11:50:55 +0200, Tomas Vondra <tomas.von...@2ndquadrant.com> wrote in <ce3cf95a-4751-c168-54ae-636c486e0...@2ndquadrant.com> > > > On 08/01/2018 01:40 PM, Emre Hasegeli wrote: > >> Ah, so there's an assumption that NaNs are handled earlier and never > >> reach > >> this place? That's probably a safe assumption. I haven't thought about > >> that, > >> it simply seemed suspicious that the code mixes direct comparisons and > >> float8_mi() calls. > > The comparison functions handle NaNs. The arithmetic functions handle > > returning error on underflow, overflow and division by zero. I > > assumed we want to return error on those in any case, but we don't > > want to handle NaNs at every place. > > > >> Not sure, I'll leave that up to you. I don't mind doing it in a > >> separate > >> patch (I'd probably prefer that over mixing it into unrelated patch). > > It is attached separately. > > > > OK, thanks. > > So, have we reached conclusion about all the bits I mentioned on 7/31? > The delta and float8/double cast are fixed, and for computeDistance > (i.e. doing comparisons directly or using float8_lt), the code may > seem a bit inconsistent, but it is in fact correct as the NaNs are > handled elsewhere. That seems reasonable, but perhaps a comment > pointing that out would be nice.
I'm not confident on replacing double to float8 partially in gist code. After the 0002 patch applied, I see most of problematic usage of double or bare arithmetic on dimentional values in gistproc.c. > static inline float > non_negative(float val) > { > if (val >= 0.0f) > return val; > else > return 0.0f; > } It is used as "non_negative(overlap)", where overlap is float4, which is calculated using float8_mi. Float4 makes sense only if we need to store a large number of it to somewhere but they are just working varialbles. Couldn't we eliminate float4 that doesn't have a requirement to do so? regards. -- Kyotaro Horiguchi NTT Open Source Software Center