[Insight-developers] Warnings : Slicer & ITK
Luis Ibanez
luis.ibanez at kitware.com
Tue Apr 28 10:18:29 EDT 2009
Hi Tom,
Thanks for pointing out these warnings.
Historical Note:
-------------------
They are the result of one sloppy practice followed by a superficial review.
The filter in question is comparing the pixel of a Kernel against "0",
and using the symbol "0" instead of the proper:
itk::NumericTraits< KernelPixelType >::Zero
which would have generated a Zero value stored in the correct type,
and which is the correct practice of Generic Programming.
because this was a comparison against "0", when the concept checks
where added. It looked like we needed a concept check for
KernelPixelType comparable to "int"
while in practice we only needed a concept check for:
KernelPixelType comparable to KernelPixelType
---
I'm now fixing the concept check and the comparison expressions
in this family of morphological filters.
Should be able to commit the fixes before the end of the morning.
Thanks
Luis
----------------------------------------------------------------------
On Tue, Apr 28, 2009 at 4:45 AM, Tom Vercauteren
<tom.vercauteren at gmail.com> wrote:
> Hi Luis,
>
> While we are at it, I would also like to report the following compiler
> warning that I keep having from ITK when I instantiated a
> GrayscaleDilateImageFilter with unsigned int images:
>
> itk::GrayscaleDilateImageFilter<
> itk::Image<unsigned int, 2u>,
> itk::Image<unsigned int, 2u>,
> itk::BinaryBallStructuringElement<
> unsigned int, 2u, itk::NeighborhoodAllocator<unsigned int>
> >
> >
>
>
> The warning I get is:
>
> itkConceptChecking.h:238: warning: comparison between signed and
> unsigned integer expressions
>
>
> This is triggered by the following concept check in GrayscaleDilateImageFilter:
>
> itkConceptMacro(KernelGreaterThanIntCheck,
> (Concept::GreaterThanComparable<typename TKernel::PixelType, int>));
>
>
> I am not sure how to fix this warning and if it makes sense to fix.
> That is, is the concept check really telling me that I am doing
> something stupid when I instantiated a GrayscaleDilateImageFilter with
> unsigned int images?
>
> Regards,
> Tom
>
> On Tue, Apr 28, 2009 at 04:14, Luis Ibanez <luis.ibanez at kitware.com> wrote:
>> Hi Steve,
>>
>>
>> Thanks for the compilation line. It seems that the
>> relevant flag is -Wall, and we have several gcc builds
>> with this flag on.
>>
>> As you pointed out, the most likely cause is that this is
>> a case that is not being instantiated in the ITK testing suite.
>>
>> I'll try to reproduce the warning in an ITK example.
>>
>> It may actually be that two incompatible concepts
>> are being confounded in these filters...
>>
>>
>> Thanks,
>>
>>
>> Luis
>>
>>
>> ------------------
>> On Mon, Apr 27, 2009 at 8:56 PM, Steve Pieper <pieper at bwh.harvard.edu> wrote:
>>> Hi Luis -
>>>
>>> Thanks for your help with this.
>>>
>>> Here's the code in question:
>>>
>>> http://viewvc.slicer.org/viewcvs.cgi/trunk/Applications/CLI/OtsuThresholdSegmentation.cxx?rev=9136&view=markup
>>>
>>> I believe it is actually an ITK example program that Bill modified a bit.
>>> It looks to me like the types are actually correct - InternalImageType is
>>> unsigned long, which is the output of the ConnectedComponents filter. So
>>> this doesn't fall into your 'risky' case.
>>>
>>> I guess this was a case that wasn't covered with the existing ITK tests.
>>>
>>> The compiler version, compile line and sample error messages are below.
>>>
>>> Best,
>>> Steve
>>>
>>>
>>> % g++ --version
>>> g++ (GCC) 4.1.2 20070925 (Red Hat 4.1.2-27)
>>>
>>> [ 93%] Building CXX object
>>> Applications/CLI/CMakeFiles/OtsuThresholdSegmentation.dir/OtsuThresholdSegmentation.cxx.o
>>> cd /workspace/pieper/slicer3/Slicer-3-4/Slicer3-build/Applications/CLI &&
>>> /usr/bin/g++ -ftemplate-depth-50 -Wall -Wno-deprecated
>>> -ftemplate-depth-50 -ftemplate-depth-50 -Wall -ftemplate-depth-50 -Wall -g
>>
>> .....
>>>
>>> Luis Ibanez wrote:
>>>>
>>>> Hi Steve,
>>>>
>>>> Thanks for pointing this out.
>>>>
>>>> --
>>>>
>>>> Fixes for the warnings that you identified in ITK
>>>> have now been committed:
>>>>
>>>>
>>>> http://www.itk.org/cgi-bin/viewcvs.cgi/Code/BasicFilters/itkConnectedComponentImageFilter.h?root=Insight&r1=1.22&r2=1.23&sortby=date
>>>>
>>>> http://www.itk.org/cgi-bin/viewcvs.cgi/Code/BasicFilters/itkConnectedComponentImageFilter.txx?root=Insight&r1=1.31&r2=1.32&sortby=date
>>>>
>>>> http://www.itk.org/cgi-bin/viewcvs.cgi/Code/BasicFilters/itkRelabelComponentImageFilter.txx?root=Insight&r1=1.17&r2=1.18&sortby=date
>>>>
>>>> http://www.itk.org/cgi-bin/viewcvs.cgi/Code/BasicFilters/itkRelabelComponentImageFilter.h?root=Insight&r1=1.16&r2=1.17&sortby=date
>>>>
>>>>
>>>> The changes are a bit different of what is proposed in your patch.
>>>>
>>>> Instead of just adding the static_casts, we have introduced additional
>>>> typedefs to clarify the use of the two distinct concepts:
>>>>
>>>> a) LabelType of a connected component.
>>>> b) ObjectSizeType of a connected component.
>>>>
>>>>
>>>> I'm curious about why these warnings didn't show up in the
>>>> ITK Dashboard...
>>>>
>>>> One possibility may be that Slicer (or Slicer Modules) may be
>>>> instantiating connected component filters by using images of
>>>> pixel type different than "unsigned long". Although it is
>>>> admissible to do so, it is also risky, specially if the pixel
>>>> type in question doesn't really has the capacity for counting
>>>> all the potential connected components that are present in
>>>> the image or that can be generated during the intermediate
>>>> procedure of labeling.
>>>>
>>>>
>>>> --
>>>>
>>>> Could you share with use the Warning flags that you are using ?
>>>> (and remind us of what platform pointed out these warnings)
>>>>
>>>> We will set them in one of the similar builds in the ITK Dashboards.
>>>>
>>>>
>>>> ---
>>>>
>>>>
>>>> About porting these changes back to ITK 3.10, we rather avoid this.
>>>>
>>>>
>>>> We only port back fixes for bugs, and only do it for the most recent
>>>> release branch. In this case ITK 3.12.
>>>>
>>>>
>>>> On the Bright side, ITK 3.14 will be out by the end of May.
>>>>
>>>>
>>>> Please let us know if you find any other problems,
>>>>
>>>>
>>>> Thanks
>>>>
>>>>
>>>>
>>>> Luis
>>>>
>>>>
>>>>
>>>>
>>>> ---------------------
>>>> Steve Pieper wrote:
>>>>>
>>>>> Hi -
>>>>>
>>>>> Remember Bill's graffiti wisdom?
>>>>>
>>>>>
>>>>> http://www.na-mic.org/Wiki/index.php/2009_Winter_Project_Week_Compiler_Warnings:Slicer3_Graffiti
>>>>>
>>>>> Well, I took it to heart and cleaned up the remaining compile warnings in
>>>>> slicer3 (trunk and 3.4 release branch). *Except* for a few lingering ones
>>>>> that trace back to ITK.
>>>>>
>>>>> The following changes are against ITK 3.12. It would also be great if
>>>>> the same fixes (and the other recent fixes to this class) could go into ITK
>>>>> 3.10 since that's what we're using for slicer3.4.
>>>>>
>>>>> If we do this, then people who download and build the release branch
>>>>> would get 0 warnings and 0 errors, and we'd all like that, wouldn't we?
>>>>>
>>>>> Thanks,
>>>>> Steve
>>>>>
>>>>> p.s. please also doublecheck that the changes make sense - they work for
>>>>> my test cases and I believe the are correct.
>>>>>
>>>>>
>>>>> % cvs -q diff
>>>>> Index: Code/BasicFilters/itkConnectedComponentImageFilter.txx
>>>>> ===================================================================
>>>>> RCS file:
>>>>> /cvsroot/Insight/Insight/Code/BasicFilters/itkConnectedComponentImageFilter.txx,v
>>>>> retrieving revision 1.31
>>>>> diff -r1.31 itkConnectedComponentImageFilter.txx
>>>>> 600c600
>>>>> < m_Consecutive[m_BackgroundValue] = m_BackgroundValue;
>>>>> ---
>>>>> > m_Consecutive[static_cast<unsigned long>(m_BackgroundValue)] =
>>>>> static_cast<unsigned long>(m_BackgroundValue);
>>>>> Index: Code/BasicFilters/itkRelabelComponentImageFilter.txx
>>>>> ===================================================================
>>>>> RCS file:
>>>>> /cvsroot/Insight/Insight/Code/BasicFilters/itkRelabelComponentImageFilter.txx,v
>>>>> retrieving revision 1.17
>>>>> diff -r1.17 itkRelabelComponentImageFilter.txx
>>>>> 105c105
>>>>> < mapIt = sizeMap.find( inputValue );
>>>>> ---
>>>>> > mapIt = sizeMap.find( static_cast<unsigned long>(inputValue) );
>>>>> 109,110c109,110
>>>>> < initialSize.m_ObjectNumber = inputValue;
>>>>> < sizeMap.insert( MapValueType( inputValue, initialSize ) );
>>>>> ---
>>>>> > initialSize.m_ObjectNumber = static_cast<unsigned
>>>>> long>(inputValue);
>>>>> > sizeMap.insert( MapValueType( static_cast<unsigned
>>>>> long>(inputValue), initialSize ) );
>>>>> 213c213
>>>>> < outputValue =
>>>>> static_cast<OutputPixelType>(relabelMap[inputValue]);
>>>>> ---
>>>>> > outputValue =
>>>>> static_cast<OutputPixelType>(relabelMap[static_cast<unsigned
>>>>> long>(inputValue)]);
>>>>> _______________________________________________
>>>>> NAMIC-Eng mailing list
>>>>> NAMIC-Eng at na-mic.org
>>>>> http://public.kitware.com/cgi-bin/mailman/listinfo/namic-eng
>>>>>
>>>
>> _______________________________________________
>> Powered by www.kitware.com
>>
>> Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html
>>
>> Please keep messages on-topic and check the ITK FAQ at: http://www.itk.org/Wiki/ITK_FAQ
>>
>> Follow this link to subscribe/unsubscribe:
>> http://www.itk.org/mailman/listinfo/insight-developers
>>
>
More information about the Insight-developers
mailing list