3 May
2012
3 May
'12
6:34 p.m.
On 16/04/2012 12:06, Edwin Carlinet wrote:
* mln/algebra/vec.hh,
* mln/value/int_u.hh,
* mln/value/rgb.hh: Interopability with different quantizations.
IMHO, that kind of changes should be discussed beforehand, and applied
uniformly to all data types. For instance, we probably want
mln::value::int_u<> and mln::value::int_s<> to be similar (at least with
respect to this type of behavior).
diff --git a/milena/mln/algebra/vec.hh
b/milena/mln/algebra/vec.hh
index 22ebc02..598c318 100644
--- a/milena/mln/algebra/vec.hh
+++ b/milena/mln/algebra/vec.hh
@@ -198,6 +198,8 @@ namespace mln
vec& operator=(const literal::origin_t&);
/// \}
+ vec(const literal::one_t&);
For symmetry grounds, the corresponding assignment operator should be
defined as well. And both should be documented.
[...]
@@ -474,6 +483,8 @@ namespace mln
return *this;
}
+
+
template<unsigned n, typename T>
inline
vec<n,T>::vec(const vec<n,T>& rhs)
Were these additions of blank lines really intended? IMHO, they break
the current coding style/logic used in this file.
diff --git a/milena/mln/value/int_u.hh
b/milena/mln/value/int_u.hh
index f3786f9..bf0b139 100644
--- a/milena/mln/value/int_u.hh
+++ b/milena/mln/value/int_u.hh
@@ -181,6 +181,12 @@ namespace mln
int_u& operator=(const mln::literal::one_t&);
/// \}
+ template<unsigned m>
+ int_u(const int_u<m>& other)
+ {
+ this->handle_() = static_cast<enc_>(other.to_enc());
+ }
This is wrong on several levels:
1. A method's definition should be separated from its declaration.
2. This ctor enables implicit (silent) conversions between int_u<>'s
with different quantization, which is not desirable, as it may be used
unintentional (error prone). I think this ctor should be marked
`explicit' at the very least, but forcing an explicit conversion is
better IMHO.
@@ -262,6 +268,15 @@ namespace mln
to_ = static_cast<double>(from);
}
+ template<unsigned n, unsigned n2>
+ inline
+ void
+ from_to_(const value::int_u<n>& from, value::int_u<n2>&
to_)
+ {
+ to_ = static_cast<unsigned>(from);
+ }
I am not sure `unsigned' is the best container for this conversion. We
probably want something like `int_u<mln_max(n,n2)>` or maybe `unsigned
long'.
+
+
} // end of namespace mln::convert::over_load
Same comment as above regarding blank lines.
diff --git a/milena/mln/value/rgb.hh
b/milena/mln/value/rgb.hh
index c9d2f53..d1729a7 100644
--- a/milena/mln/value/rgb.hh
+++ b/milena/mln/value/rgb.hh
@@ -283,7 +283,9 @@ namespace mln
/// Constructor from a algebra::vec.
rgb<n>(const algebra::vec<3, int>& rhs);
rgb<n>(const algebra::vec<3, unsigned>& rhs);
- rgb<n>(const algebra::vec<3, int_u<n> >& rhs);
+
+ template<unsigned m>
+ rgb<n>(const algebra::vec<3, int_u<m> >& rhs);
rgb<n>(const algebra::vec<3, float>& rhs);
// Conversion to the interoperation type.
@@ -316,10 +318,16 @@ namespace mln
/// \}
/// Assignment.
+ template<unsigned m>
+ rgb<n>& operator=(const rgb<m>& rhs);
+
I think we should discuss these changes as well.
rgb<n>& operator=(const
rgb<n>& rhs);
/// Zero value.
static const rgb<n> zero;
+
+ // friend class access
+ template<unsigned> friend struct rgb;
};
Is this really needed? We should avoid using `friend'.
[...]
@@ -650,6 +671,8 @@ namespace mln
return *this;
}
+
+
Ditto.