Re: rewrite of structs - advice sought

[John W. Eaton]

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.

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.

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);

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.

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.

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?

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));

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

  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;

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.

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 ());
  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);

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;

The actual type of the iterators is now different, but do we have any
code that actually depends on the details of the typedef?  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);

I'm not sure about the following difference.

  NEW:  const_iterator begin (void) const;
  OLD:  const_iterator begin (void);

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);

jwe