On Thu, Nov 12, 2009 at 5:37 PM, John W. Eaton
I think it would be preferable if you posted a diff for discussion
before comitting large changes that completely alter the structure of
the way things are done. In most cases, we will probably agree that
the change is good and can be applied, but I think it would be better
to have some discussion before comitting.
| Rationale:
| Until now, there existed a single virtual octave_base_value method for each
| mapper.
The current method of doing things has only been in place since mid
February of 2008. There was a discussion about it at the time, before
the change was made, here:
https://www-old.cae.wisc.edu/pipermail/octave-maintainers/2008-February/006013.html
The initial changeset is here:
http://hg.savannah.gnu.org/hgweb/octave/rev/8c32f95c2639
| Now, there is a single map (unary_mapper_t) method that takes an
| enum parameter identifying the mapper. The new style has some advantages:
| 1. It becomes far easier to implement a "handle some, convert & forward
| others" approach.
Can you give a specific example of this?
| 2. When adding a new mapper, less classes need to be touched. Adding support
| for scalars and dense arrays mostly suffices; the fallback conversions will
| handle the rest.
Where is the conversion happening before, and now with your change?
With the previous code, could conversion have been handled by the
octave_base_value mapper functions?
| 3. Adding a new mapper won't alter the VMT, hence the ABI is not
| broken.
How often are new mapper functions added? Do you have plans to add some?
| 4. 40 virtual methods are replaced by one, saving 39 VMT entries :)
| (probably not a real issue though)
It seems to me that you have traded a set of virtual functions for a
collection of large switch statements. Usually, I would want to go in
the other direction and replace large switch statements with virtual
functions, but I might agree with the change if there are some good
reasons.
| I think the only disadvantage is that one now needs to write value.map
| (umap_abs) rather than value.abs (). If anyone thinks that would be a good
| idea, the old mappers can be put back to octave_value for compatibility.
It might be more natural to write value.abs() if you are converting
some script to C++. Maybe it would also be good to hide the enum
details and leave the interface at the octave_value level alone?
| I also altered somewhat the way mapping functions on arrays is handled, but
| that's even more technical and probably not yet final...
What changes are you referring to here?
| Of those, 30 are caused by the fact that isalpha, tolower et al. now don't
| work for numeric inputs. Matlab apparently only has lower/upper and only
| mentions characters. Giving an error on, say, toupper (1+1i) makes sense to
| me, so unless someone disagrees I'll alter the tests instead.
I wouldn't be surprised if there is some old Matlab code out there
that relies on numeric (probably not complex) values in the range of
ascii characters behaving like characters.