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.