[Insight-developers] Iterators & "operator *"

[Luis Ibanez]

Hi Gert,


1) Yes, I do like those 'const'  :-)

    Also,
    I'm quit biased due to the asymmetric situation that is very easy
    to remove a 'const', and it is very hard to add a 'const'.

    Let's do this,
    Could you please log a bug report regarding this issue ?

    In this way we will be able to keep track of the discussion,
    and will make sure that we don't forget about it.


2) I still disagree with the notion that calling Set() "changes
    the iterator".


    My point is that if "p" is a pointer pointing to "q", then
    the following code:

       int q;

       int * p = &q;

       *p = 5;

    doesn't changes "p", the pointer.

    it changes "q", the content at which the pointer "p" points.


3) About the ImportImageFilter,

    Yes, if you "manually" access pixel data at
    the output of the ImportImageFilter, then
    you may run into many problem.

    Note that, in general, the pipeline architecture
    of ITK states that you shouldn't modify the pixel
    data or the metadata of an image that is the output
    of a filter, without first disconnecting the image
    from the filter that produced it.

    Note that "managing" the memory of the original
    buffer is optional, and it is set via a boolean.

    We describe multiple strategies for importing data
    into ITK images in the Tutorial:

http://www.itk.org/CourseWare/Training/GettingStarted-V.pdf


4) If we follow the path of deriving STL-compatible
    image iterators from the existing image iterators,
    you will be able to overload the != method and to
    implement it in a consistent way.

    In practice, in ITK, we rarely use the != method
    between iterators. The standard procedure is to
    use the bool IsAtEnd() method. This is possible
    because the iterators knows a lot about the internal
    implementation of the image.

    Adding the std::iterator at the base of an ITK-STL-compatible
    family of iterators, sounds like a good idea. In this case
    you could have the ITK-STL iterators to "have" an ITK iterator
    inside, and could delegate the ++ operation from the ITK-STL
    iterator to the existing ++ operation of the internal iterator.

    As you pointed out, in this way you get most of the necessary
    traits from the base STL class.


5) I definitely agree with you in that the heaviest part
    of the task will be to define a proper set of test.




     Luis



-------------------
Gert Wollny wrote:
> You really like that 'const' don't you ;-) 
> 
>>1) One motivation for a const iterator (one that can't iterate)
>>    would be to have the "begin" iterator, and/or the "end"
>>    iterator. E.g. iterators that are only provided for comparison
>>    with others, and therefore do not need to iterate themselves.
> 
> That doesn't count: On one hand, if it is only used for comparison, why
> would you want to call the Set() method, there is still this other
> iterator you're comparing to to do so. On the other hand, specifically
> for the "end" iterator, you wouldn't want to call Set() anyway, right?
> 
> 
>>2) One alternative to casting away the const in the Set() method
>>    is to declare mutable the variables that we are using for
>>    changing the content of the image. We may have to look at this
>>    closely to figure out if it is a wise option, since mutable
>>    is sometimes abused as a lazy short-cut for more fundamental
>>    const-correctness design problems.
> 
> What my logical problem is: If I call only const member functions in a
> row, e.g. 
> 
>    struct A {
> 	int f1() const; 
> 	int f2(int v) const; 
>    } i; 
> 
>    a=i.f1(); b=i.f2(v); c=i.f1(); 
> 
> then 'const' suggests to me, that a==c (multi-threading and volatile
> members aside). 
> Back to  the iterators, I don't have to know how it is implemented and
> what ist does internally. I only need to know, after calling Set(),
> Get() most likely will return a different value, and therefore, the
> iterator changed logically, and this makes the const declaration really
> difficult to accept for me, other might think different.
> 
> 
>>3) About memory overlap on two images, you can find this situation
>>    when using the ImportImageFilter. E.g. when creating ITK images
>>    from pre-existing pixel buffers.
> 
> After a short look at the code ... Does this mean, since
> the ImportImageFilter manages the memory of these images, something
> like
> 
> Image::Pointer get_image(...) 
> {
>   ImportImageFilter<...> Filter f=Filter::New(); 
>   ...
>   f->Update(); 
>   return f->Output(); 
> }
> void process() 
> {
>   ...
>   Image::Pointer i = get_image(...); 
>   i->DoSomethingWithThePixels(); 
>   ...
> }	
> 
> will result in a crash at DoSomethingWithThePixels(), because the
> memory is already freed when f is released at the end of get_image()? 
> Maybe it's better to copy the data, or ImportImageFilter doesn't manage
> the memory at all and it's documented that the user has to take care of
> it?
> 
> 
>>    That being said, you have a point in that we should review
>>    the definition of the != operator, by taking into account
>>    all the realistic use cases of the operator.
>>
>>    We should also revisit the operator with the mindset that
>>    if it is used in an STL style, the actual method must be
>>    reduced to as few operations as possible.
> 
> Having the usual use of STL iterators in mind and, specifically, the
> frequent use of != as loop terminating condition, comparing iterators
> with different base pointers should either result in an assertion
> failing, because they would never be equal and, hence, comparing them
> would constitute a bug, or the sum (base+offset) is kept, because we
> allow constructs with memory-overlapping image areas.
> 
> 
> 
>>4) A suggestion could be to add to ITK a set of STL compatible
>>    iterators. They could simply derive from existing iterators
>>    and add the operator*(), traits and any other API required
>>    by STL iterators.
>>
>>    In this way, the toolkit could become accessible to STL
>>    algorithms without compromising the existing API.
> 
> The least amount of additional code is created by making the
> std::iterator the base of the tree and add "*() const" in the
> ConstIterator and *() in the Iterator. The interface will only be
> extended orthogonal, not changed. I think the main work will be a proper
> set of tests. 
> 
> Best, 
> 
> Gert 
> 
> 
>