[Xapian-tickets] [Xapian] #481: Merge geospatial branch

Xapian nobody at xapian.org
Thu Jan 12 22:22:04 GMT 2012


#481: Merge geospatial branch
-------------------------+--------------------------------------------------
 Reporter:  richard      |       Owner:  richard  
     Type:  enhancement  |      Status:  assigned 
 Priority:  highest      |   Milestone:  1.3.0    
Component:  Library API  |     Version:  SVN trunk
 Severity:  normal       |    Keywords:           
Blockedby:               |    Platform:  All      
 Blocking:               |  
-------------------------+--------------------------------------------------

Comment(by olly):

 {{{LatLongCoord::operator<}}} does:

 {{{
 #!cpp
     bool operator<(const LatLongCoord & other) const
     {
         if (latitude < other.latitude) return true;
         return (longitude < other.longitude);
     }
 }}}

 I think this probably should have a middle line of:

 {{{
 #!cpp
         if (latitude > other.latitude) return false;
 }}}

 As it stands, if a has lat 10, long 20 and b has lat 20, long 10 then both
 {{{a < b}}} and {{{b < a}}} are true, which is (a) surprising and (b) I
 believe leads to undefined behaviour with the STL.  Is this just an
 oversight?  Presumably this method exists so that they can be put in
 containers like std::map, so the actual ordering is mostly arbitrary, so
 long as it's consistent.

 A bit unsure about having a naked struct for !LatLongCoord - we've avoided
 this elsewhere in the API.

 The unserialise() methods for !LatLongCoord and !LatLongCoords work
 differently to our existing unserialise methods, which all return a new
 object or object pointer.  Perhaps that's sensible,
 but the inconsistency grates a bit.

 There's a documented return value here for a (private) method which
 actually returns void:

 {{{
 #!cpp
     /** Calculate the distance for the current document.
      *
      *  Returns true if the distance was calculated ok, or false if the
      *  document didn't contain a valid serialised sequence of coordinates
 in
      *  the appropriate value slot.
      */
     void calc_distance();
 }}}

 LatLongDistanceKeyMaker's ctors lack documentation comments.

 10E10 seems a bit of an odd default value (1e11 seems a more natural way
 to write the
 same thing).  But perhaps we should just split the ctors and initialise
 {{{defkey(9, '\xff')}}} when no default distance is specified (which is
 more efficient
 than defaulting the distance to HUGE_VAL and then converting it, and
 avoids pulling in
 <cmath> in an API header).

 I've also fixed a pile of documentation comment typos in my local tree.
 I'll try to sort
 out a merge request for changes.

-- 
Ticket URL: <http://trac.xapian.org/ticket/481#comment:27>
Xapian <http://xapian.org/>
Xapian



More information about the Xapian-tickets mailing list