Re: rewrite of structs - advice sought

[Jaroslav Hajek]

On Tue, Jun 22, 2010 at 10:49 PM, John W. Eaton <address@hidden> wrote:
> On 22-Jun-2010, Jaroslav Hajek wrote:
>
> | On Tue, Jun 22, 2010 at 5:20 PM, John W. Eaton <address@hidden> wrote:
> | > On 22-Jun-2010, Jaroslav Hajek wrote:
> | >
> | > | 1. general opinions about this? Is this a worthy goal for Octave? So
> | > | far it had been mostly fun, now the hard work (debugging) begins, so
> | > | I'd like to know if I should abandon the effort in time.
> | > |
> | > | 2. naming: I chose octave_map and octave_scalar_map, so that
> | > | octave_map and Octave_map only differ in capitalization. Since
> | > | Octave_map should eventually go away, this almost-nameclash seemed OK
> | > | to me, but I'm open to objections.
> | >
> | > Is there a less radical set of changes that could be made that would
> | > improve performance but leave the existing interface while not having
> | > two internal implementations?
> |
> | I don't know. I initially considered having the polymorphism inside
> | octave_map directly, but it was getting clumsy. In most other cases,
> | data is polymorphic at octave_value level as well (with the exception
> | of idx_vector).
> |
> | Having a simpler, specialized class for the scalar struct is actually
> | a key idea. It should be easier and more efficient to work with, e.g.
> | use m.contents (key) instead of m.contents (key)(0) etc.
> |
> | > I guess I don't see why you need to
> | > completely replace the old interface with something new.
> |
> | Because the data structure was completely different.
>
> Sure, but I don't see why the class interface must change completely
> as well.
>

Oh, but it didn't change completely, as you note below.

> Here are the similarities and differences that I see for the
> two interfaces.  I generated this list by looking at the declarations
> for the public methods in the Octave_map and octave_map classes in
> your new oct-map.h file and prefixing the Octave_map method
> declarations with OLD: and the octave_map method declarations with
> NEW:, changing all occurrences of Octave_map to octave_map, then
> sorting and comparing.  With the exception of the differences in the
> actual types of the iterators, I don't see any reason that the old
> interface can't be preserved, such that a simple
>
>  typedef octave_map Octave_map;
>
> will allow the old class name to continue to work while using the new
> internals.
>
> The following methods appear in both the old Octave_map and the
> new octave_map classes with exactly the same interface:
>
>  Cell contents (const std::string& k) const;
>  Cell& contents (const std::string& k);
>  Cell& contents (iterator p);
>  const_iterator end (void) const;
>  const_iterator seek (const std::string& k) const;
>  dim_vector dims (void) const;
>  int ndims (void) const;
>  octave_idx_type columns (void) const;
>  octave_idx_type nfields (void) const;
>  octave_idx_type numel (void) const;
>  octave_idx_type rows (void) const;
>  octave_map (const octave_map& m);
>  octave_map (const octave_map& m);
>  octave_map concat (const octave_map& rb, const Array<octave_idx_type>& 
> ra_idx);
>  octave_map permute (const Array<int>& vec, bool inv = false) const;
>  octave_map reshape (const dim_vector& dv) const;
>  octave_map squeeze (void) const;
>  octave_map transpose (void) const;
>  octave_map& operator = (const octave_map& m);
>  std::string key (const_iterator p) const;
>  void clear (void);
>
> The following appear only in the new octave_map class.  Additions to
> the old interface should not cause any trouble.
>
>  Cell getfield (const std::string& key) const;
>  Cell& contents (octave_idx_type i);
>  const Cell& contents (octave_idx_type i) const;
>  octave_idx_type cols (void) const;
>  octave_idx_type index (const_iterator p) const;
>  octave_idx_type length (void) const;
>  octave_map index (const Array<idx_vector>& ia, bool resize_ok) const;
>  octave_map index (const idx_vector& i, bool resize_ok) const;
>  octave_map index (const idx_vector& i, const idx_vector& j, bool resize_ok) 
> const;
>  octave_map orderfields (Array<octave_idx_type>& perm) const;
>  octave_map orderfields (const octave_map& other, Array<octave_idx_type>& 
> perm) const;
>  octave_map orderfields (void) const;
>  octave_scalar_map checkelem (const Array<octave_idx_type>& ra_idx) const;
>  octave_scalar_map checkelem (octave_idx_type i, octave_idx_type j) const;
>  octave_scalar_map checkelem (octave_idx_type n) const;
>  static octave_map cat (int dim, octave_idx_type n, const octave_map 
> *map_list);
>  static octave_map cat (int dim, octave_idx_type n, const octave_scalar_map 
> *map_list);
>  void delete_elements (const Array<idx_vector>& ia);
>  void delete_elements (const idx_vector& i);
>  void delete_elements (int dim, const idx_vector& i);
>
> There is no destructor declared in the new class, but the old one was
> trivial anyway, so that doesn't matter.
>
> The following methods appear to have only changed names:
>
>  NEW:  string_vector fieldnames (void) const;
>  OLD:  string_vector keys (void) const;
>
>  NEW:  void rmfield (const std::string& key);
>  OLD:  void del (const std::string& k);
>
>  NEW:  bool isfield (const std::string& name) const;
>  OLD:  bool contains (const std::string& k) const;
>
> I don't object to the new names, but it looks like the old ones could
> be trivially preserved.  I also don't object to marking the old names
> as deprecated.
>

Yes, I intended to do this later, they're not there yet so that I can
find and fix usages more easily. The old names seemed a little ad hoc
to me, I tried to use the closes built-in function name instead. I
don't even think those need to be deprecated.


> Except for the difference in return values, do the following peform
> the same task?
>
>  NEW:  void setfield (const std::string& key, const Cell& val);
>  OLD:  octave_map& assign (const std::string& k, const octave_value& rhs);
>

No.
assign (const std::string& k, const Cell& rhs) did the same task. I
think it's better if the field access and indexing don't have
identical names, so that they're more separated. This has confused me
a few times when reading Octave's code.

> If so, then it should be trivial to preserve this assignment function
> using the new setfield function.  Again, I would not object to marking
> the old function as deprecated.
>

I'm OK with adding the Cell version (and marking as deprecated), but I
think the octave_value version should not be in octave_map. It should
be in octave_scalar_map where it logically belongs (because it only
works with scalar structs), or not exist at all. Also, existence of
both in Octave_map is an annoying ambiguity, for instance, if you want
to pass Array<octave_value> as val.

> The following interface changed in the new octave_map class:
>
>  NEW:  void assign (const octave_value_list&, const octave_map& rhs);
>  OLD:  octave_map& assign (const octave_value_list& idx, const octave_map& 
> rhs);
>
> Is there any reason for the assign method not to return a reference to
> *this?  I doubt that much (if any) code currently depends on it, so I
> would not be opposed to dropping it.  But I also don't see the harm in
> having assign methods return a reference to *this.
>

AFAIK, no code in Octave depends on it. I changed it for consistency
with the other assign methods in Array and Sparse. Anyway, things like
x.assign (idx, y).whatever () seem very confusing to me.


> These assignment methods are new:
>
>  NEW:  void assign (const Array<idx_vector>& ia, const octave_map& rhs);
>  NEW:  void assign (const idx_vector& i, const idx_vector& j, const 
> octave_map& rhs);
>  NEW:  void assign (const idx_vector& i, const octave_map& rhs);
>
> And these have been removed:
>
>  OLD:  octave_map& assign (const octave_value_list& idx, const std::string& 
> k, const Cell& rhs);
>  OLD:  octave_map& assign (const std::string& k, const Cell& rhs);
>
> Is there any reason the two old assign functions can't be provided using
> the new internals?

They surely can. The second one is discussed above (equivalent to
setfield), the first one is equivalent to

m.contents (k).assign (idx, rhs);

>
> Changes in default argument values:
>
>  NEW:  octave_map (const string_vector& k);
>  OLD:  octave_map (const string_vector& sv, const dim_vector& dv = dim_vector 
> (0, 0));
>

octave_map actually provides all three combinations (keys only, dv
only, keys and dv). I think the dv comes first, but I've got no
problem with changing the order.

>  NEW:  void resize (const dim_vector& dv);
>  OLD:  void resize (const dim_vector& dv, bool fill = false);
>

I think I'll fix this one.

>  NEW:  octave_map index (const octave_value_list& idx, bool resize_ok) const;
>  OLD:  octave_map index (const octave_value_list& idx, bool resize_ok = 
> false) const;
>

Yes, the resize_ok args should default to false in all cases.

> Is there any reason the old interfaces could not be preserved?
>
> Changes in return values:
>
>  NEW:  const Cell& contents (const_iterator p) const;
>  OLD:  Cell contents (const_iterator p) const;
>
> I don't think this causes trouble.  Does it cause a binary
> incompatibility?  Even so, I don't have a problem with making this
> change.
>

This is intentional, for efficiency reasons. The iterator is supposed
to be valid, so a reference can always be returned.
It differs from contents(const std::string&).

> The following constructors from the old class interface are not
> provided directly by the new interface, but I don't see why they can't
> be preserved.
>
>  octave_map (const dim_vector& dv = dim_vector (0, 0), const Cell& key_vals = 
> Cell ());

This one is almost a duplicate of
Octave_map (const string_vector& sv, const dim_vector& dv = dim_vector (0, 0));

Also, note that the order of arguments is reversed. I find that _very_
confusing; I think the former should go away (or the order of args be
fixed, but then there's no need for it to be there).

>  octave_map (const std::string& k, const Cell& vals);
>  octave_map (const std::string& k, const octave_value& value);
>  octave_map (const std::string& k, const octave_value_list& val_list);
>

These construct a structure with just one field. That is, IMHO, almost
useless; structs with a single field are exceptionally rare. You
normally want to add more fields afterwards, and then it's quite
confusing to treat the first field differently:

octave_map m ("field1", val1);
m.setfield ("field2", val2);
m.setfield ("field3", val3);

vs.

octave_map m;

m.setfield ("field1", val1);
m.setfield ("field2", val2);
m.setfield ("field3", val3);


> The following functions are provided as a convenience for extracting a
> single int or string value from a structure.  I think they could be
> preserved.
>
>  OLD:  int intfield (const std::string& k, int def_val = 0) const;
>  OLD:  std::string stringfield (const std::string& k, const std::string& 
> def_val = std::string ()) const;
>

Well, yes, but again, these only make sense for scalar_map. I was, in
fact, thinking about implementing something general using a template,
like

template <class TYPE>
TYPE getfield (const std::string& k, const TYPE& def_val = TYPE ())
{
  octave_value v = getfield (k);
  return v.is_defined () ? octave_value_extract<TYPE> (v) : def_val;
}

> The actual type of the iterators is now different, but do we have any
> code that actually depends on the details of the typedef?

Yes, we have. The preferable way to access the iterator's key and
value is through octave_map::contents and octave_map::key, but on
several places the internal members of std::map::iterator were
accessed directly. Those needed to be fixed.

> Or is it
> sufficient that the class provides const and non-const iterators with
> the names iterator and const_iterator?
>
>  NEW:  typedef const_iterator iterator;
>  NEW:  typedef octave_fields::const_iterator const_iterator;
>
>  OLD:  typedef std::map<std::string, Cell>::const_iterator const_iterator;
>  OLD:  typedef std::map<std::string, Cell>::iterator iterator;
>
> The following iterators appear to be used only internally even though
> they are declared public, so eliminating them should not cause trouble.
>
>  OLD:  typedef std::list<std::string>::const_iterator const_key_list_iterator;
>  OLD:  typedef std::list<std::string>::iterator key_list_iterator;
>
> Since iterator and const_iterator are the same in the new class, and
> const versions of the following are provided, that we don't need to
> explicitly define the non-const versions that were in the old
> interface?
>
>  OLD:  iterator begin (void);
>  OLD:  iterator end (void);
>  OLD:  iterator seek (const std::string& k);
>

Yes, this is useless now. const_iterator and iterator are the same
stuff because their contents can no longer be accessed directly, and
there is nothing else what they need to differ in. If we wanted to
preserve the direct access via p->first and p->second, special classes
would need to be coded for each iterator (with overloaded ->
operator). I'd rather avoid that, I like the key/contents approach
better, it's more general.

> I'm not sure about the following difference.
>
>  NEW:  const_iterator begin (void) const;
>  OLD:  const_iterator begin (void);
>

No harm in this direction, though. I actually didn't understand why
the old method was non-const.

> The following function has changed names and return type, but I don't
> see why the old interface can't be preserved.
>
>  NEW:  void delete_elements (const octave_value_list&);
>  OLD:  octave_map& maybe_delete_elements (const octave_value_list& idx);
>

Consistency again. See Array::delete_elements. I never understood why
the "maybe", btw.

I recall you said once you preferred consistency and good code over
backward compatibility. The above discussion makes it clear that 100%
compatibility between octave_map and Octave_map is not achievable
without sacrificing consistency and logic, although we can do better
than the current code (which I'll attempt to do).

It still seems to me that it would be better to leave Octave_map in
place and simply deprecate it as whole, so that developers become
aware of the changes and choose the more suitable replacement. Most of
them will want to use the scalar map anyway (some people don't even
know structs are arrays :).

regards

-- 
RNDr. Jaroslav Hajek, PhD
computing expert & GNU Octave developer
Aeronautical Research and Test Institute (VZLU)
Prague, Czech Republic
url: www.highegg.matfyz.cz