On Thu, Nov 12, 2009 at 5:37 PM, John W. Eaton
<address@hidden> wrote:
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.
That's the old discussion about how much "working" the development repo is. I prefer using the repo rather than sharing patches via mail. I remind that we still have the secondary "stable" repo set up on Savannah.
| 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
Thanks, I'll look at that.
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?
For instance, octave_range now only needs
octave_value map (unary_mapper_t umap) const
{
octave_matrix m (matrix_value ());
return m.map (umap);
}
previously, there were 40 separate forwards for each method. Similarly for octave_perm_matrix, diag matrices etc.
| 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?
Previously, each mapper method needed to be separately overriden for each class it was supposed to work on, so the default conversions were injected into each method, typically via a macro. Now it's only needed once as a the default: case.
With the previous code, could conversion have been handled by the
octave_base_value mapper functions?
I didn't see a good way. I thought about using somehow the default numeric conversions for operators, but that didn't seem to quite work.
| 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?
Yes. I also added some in the past (log2, roundb) and I remember that it was error-prone.
| 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 traded 40 similar virtual methods for one virtual method with a parameter. That idea is too general; here each switch statements replaced 40 methods, not one call.
| 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?
OK, good suggestion. Maybe the enum can be in octave_base_value and protected; anyway, only its descendants will probably need it.
| 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?
Array<T>::map was repeatedly overriden in subclasses, hiding the original method (template method hiding) and disabling inlining possibilities. I removed most of these overrides and instead I'm accessing the methods directly, resulting in less boilerplate code.
| 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.
OK. So let's allow only real numeric args?
| I checked the sources and it seems this behaviour is intended - positive
| imaginary zeros are narrowed, negative zeros aren't.
Yes, I think that is intentional, so that you get different results
for things like
complex (-0, 1)/inf
complex (0, 1)/inf