Re: [vsipl++] [selgen]

[Jules Bergmann <jules@xxxxxxxxxxxxxxxx>]

Stefan Seefeld wrote:
> The attached patch implements all functions from section 9.4 ([selgen])
> of the spec, i.e.
>
> * first()
> * indexbool()
> * gather()
> * scatter()
> * clip()
> * invclip()
> * swap()
>
> together with unit tests. It also contains some bits and pieces I
> submitted earlier today to cleanup header dependencies etc., as I
> wasn't able to easily separate the two.
Stefan,

Looks good.  I have one suggestion for indexbool to make it a little 
more robust, otherwise it looks ready to check in.
Also, were the unit tests included in the patch?

				thanks,
				-- Jules


>  
>  /***********************************************************************
> @@ -30,6 +31,29 @@
>  namespace impl
>  {
>  
> +template <typename T, typename B1, typename B2>
> +length_type
> +indexbool(const_Vector<T, B1> source, Vector<Index<1>, B2> indices)
> +{
> +  index_type cursor = 0;
> +  for (index_type i = 0; i != source.size(); ++i)
> +    if (source.get(i))
> +      indices.put(cursor++, Index<1>(i));
> +  return cursor;
> +}
I'm trying to think if we can do better error checking here.  This 
doesn't check if cursor < indices.size(0), but the put does, so that's 
good.  It would be good to have an assertion in indexbool so that the 
failure is more obvious.
However, I don't think the specification of indexbool makes it very 
useful.  It should handle an overflow more gracefully than either 
aborting or corrupting memory.  Since the overflow condition is 
data-dependent, it forces me to size indices for the absolute worst 
case.  Hypotheticaly, if I'm doing target detection on an IR sensor and 
a flare goes off, I'm going to have way more detections for a few frames 
until I have a chance to adapt my thresholds.  As a system engineer, I 
would probably choose to drop some detections for a few frames rather 
than size my detection buffer for the absolute worst-case.  I certainly 
don't want the application to crash or corrupt itself!
This is a future opportunity here to design a better interface (such as 
a stateful one that avoids overflow by getting the next N detections 
from source, where N is the size of indices).
In the short term, let's check that cursor is less than indices.size() 
before doing the put, i.e.:
  index_type cursor = 0;
  for (index_type i = 0; i != source.size(); ++i)
    if (source.get(i) && cursor++ < indices.size())
      indices.put(cursor-1, Index<1>(i));
  return cursor;

The returned value (cursor) is still the "number of non-false values in 
source" (as required by the spec) and we avoid overwriting memory.  A 
concerned user can check if the returned value is greater than 
indices.size().

> +
> +template <typename T, typename B1, typename B2>
> +length_type
> +indexbool(const_Matrix<T, B1> source, Vector<Index<2>, B2> indices)
> +{
> +  index_type cursor = 0;
> +  for (index_type r = 0; r != source.size(0); ++r)
> +    for (index_type c = 0; c != source.size(1); ++c)
> +      if (source.get(r, c))
> +	indices.put(cursor++, Index<2>(r, c));
> +  return cursor;
> +}
Let's do the same as above.


>  
> +namespace impl
> +{
> +template <typename Tout, typename Tin1>
> +struct clip_wrapper
> +{
> +  template <typename Tin0>
> +  struct clip_functor
> +  {
> +    typedef Tout result_type;
> +    result_type operator()(Tin0 t) const 
> +    {
> +      return t <= lower_threshold ? lower_clip_value 
> +	: t < upper_threshold ? t
> +	: upper_clip_value;
> +    }
> +
> +    Tin1 lower_threshold;
> +    Tin1 upper_threshold;
> +    result_type lower_clip_value;
> +    result_type upper_clip_value;
> +  };
> +  template <typename Tin0>
> +  struct invclip_functor
> +  {
> +    typedef Tout result_type;
> +    result_type operator()(Tin0 t) const 
> +    {
> +      return t < lower_threshold ? t
> +	: t < middle_threshold ? lower_clip_value
> +	: t <= upper_threshold ? upper_clip_value
> +	: t;
> +    }
> +
> +    Tin1 lower_threshold;
> +    Tin1 middle_threshold;
> +    Tin1 upper_threshold;
> +    result_type lower_clip_value;
> +    result_type upper_clip_value;
> +  };
> +};
> +  
Why are clip_functor and invclip_functor nested in clip_wrapper?  (I'm 
just curious, I'm not suggesting that it should be changed)
> +
> +namespace impl
> +{
> +/// Generic swapping of the content of two blocks.
> +template <typename Block1, typename Block2>
> +struct Swap
> +{
> +  static void apply(Block1 &block1, Block2 &block2)
> +  {
> +    assert(block1.size() == block2.size());
> +    for (index_type i = 0; i != block1.size(); ++i)
> +    {
> +      typename Block1::value_type tmp = block1.get(i);
> +      block1.put(i, block2.get(i));
> +      block2.put(i, tmp);
> +    }
> +
> +  }
> +};
Looks good.  We can plug in specializations to Swap for things like 
swapping pointers (if we decide it's worth doing).