On 08/01/2018 04:07 PM, Emre Hasegeli wrote:
I think there are three different things that need to be addressed:
* Underspecified comments.
I agree. My patch added comments, the next one with actual fixes adds
more. I just didn't want to invest even more time on them while the
code is its current shape.
* The function names and argument names are badly chosen IMO, because even
granted a convention such as the above, it's not very obvious what roles
"l1" and "l2" play. I'm not exactly sure what would be better, but if you
used names like "ofseg" and "otherseg" you'd at least be trying. I'd go
with an asymmetrical function name too, to make miswriting of calls less
likely.
Good idea. I haven't though about that, but now such names makes more
sense to me. I will prepare another patch to improve the naming and
comments to be applied on top of my current patches.
* And lastly, are we sure there aren't actual *bugs* here? I'd initially
supposed that lseg_closept_lseg acted as Emre says above, but reading the
code makes me think it's the other way around. Its first two potential
assignments to *result are definitely assigning points on l2 not l1.
Yes, it is wrong beyond understanding. The committed patch intended
to keep answers as they were. The next one actually fixes those.
OK, so I think we should not do anything about the issues reported by
coverity until we commit all the patches. Which may resolve some of
those (pre-existing) issues, for example.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
