[Insight-developers] A massive number of new warnings

Fri Jan 7 10:54:34 EST 2011

Brad,

If the developer intended to use "UINT_MAX" as the error code,

then,

they simply should write "UINT_MAX" in the code.

and in that way they can have type consistency, proper documentation
and maintainability.

Even better,
they should have a #define in a upper file, stating:

#define   READ_RETURN_ERROR_CODE    UINT_MAX

and then use READ_RETURN_ERROR_CODE in the actual
evaluations of the code.

------

As far as whether the change
modified the behavior of the code:

       Well...
       that's why we run tests, isn't it ?

And that's why another new year's resolution should be
to raise the code coverage of important modules to 95%.

     Luis

------------------------------
On Fri, Jan 7, 2011 at 9:16 AM, Bradley Lowekamp <blowekamp at mail.nih.gov> wrote:
> Luis,
> Isn't UINT(-1) a short hand for the max unsigned int? Isn't this behavior
> well defined by the C++ standard? Could the only thing wrong with the code
> be there is just an implicit case, and not an explicit one? You may have
> changed what the code was doing.
> Brad
> On Jan 7, 2011, at 8:58 AM, Luis Ibanez wrote:
>
> Brad,
>
> I'm all for treating Warnings as errors,
> and agree with Dave's point that it is the way to
> prevent them from being dismissed by developers.
>
> In many cases they are hinting us to locate real errors.
>
> For example, in this patch:
>
> http://review.source.kitware.com/#change,679
>
> The warnings in openjpeg were revealing bugs in the
> API of many functions.
>
> In particular, functions whose return type is declared
> as UINT32, and the code of the implementations return
> "-1", and code down the line compares against "-1".
>
> http://review.source.kitware.com/#patch,sidebyside,679,1,Utilities/openjpeg/openjpeg.c
>
> Those are real bugs,
> that were hinted by warnings.
>
>
>      Luis
>
>
> ----------------------------------------------------------------------------------------
> On Thu, Jan 6, 2011 at 9:11 PM, Bradley Lowekamp <blowekamp at mail.nih.gov>
> wrote:
>
> Hello,
>
> I have a hard time getting excited about this issues when logic and valid
>
> code, which may be more optimal needs to be changes to suppress a warning
>
> only when -Wextra is enabled:
>
> http://review.source.kitware.com/#change,677
>
> There are useful warning and there are un-useful ones. At any rate, I agree
>
> that the dashboard needs some work to clean up some of these warnings. And I
>
> applaud the effort, and wish for more snow in New York to help get it done
>
> :)
>
> Brad
>
>
> On Jan 6, 2011, at 6:04 PM, David Cole wrote:
>
> On the dashboards it's not really an issue because they report warnings or
>
> errors, either one just fine.
>
> The problem is sitting in front of the keyboard... If it's not an error that
>
> prevents the build from completing, it will be ignored by *somebody*. And
>
> then, it just sneaks right in and forces those of us who monitor the
>
> dashboard health to take on the burden and the pain of cleaning it up. But
>
> if it is an error, then that same somebody has to deal with it before moving
>
> on: it'll get fixed much quicker if it's a build error for everybody who
>
> sees it.
>
> Of course, the other problem is that you can't build it yourself on *all*
>
> the platforms and different warnings show up on different platforms. So, in
>
> that respect, everybody causes a warning elsewhere once in a while through
>
> no fault of their own.
>
> But, just some food for thought: if "treat warnings as errors" were on by
>
> default for everybody's build, and hard to turn off, how many warnings would
>
> there be when compiling the source code. I'm guessing close to zero the vast
>
> majority of the time.
>
>
> ;-)
>
>
> On Thu, Jan 6, 2011 at 5:19 PM, Bill Lorensen <bill.lorensen at gmail.com>
>
> wrote:
>
> Eventually, if we can get warnings near zero, I agree that at least
>
> for our dashboards we should set the flag that treats warnings as
>
> error.
>
>
> On Thu, Jan 6, 2011 at 4:13 PM, David Cole <david.cole at kitware.com> wrote:
>
> Actually the easiest way to get the point across (and win enemies and
>
> influence people) is to set the flag that treats warnings as errors.
>
> That'll grab their attention!!
>
> (Of course, you might not be able to get past the Utilities directory
>
> in the next decade, but.....)
>
>
> :-)
>
> David
>
>
> On Thu, Jan 6, 2011 at 4:02 PM, Hans Johnson <hans-johnson at uiowa.edu>
>
> wrote:
>
> I was toying with the idea of adding something like is shown at the
>
> bottom
>
> of this message.
>
> We could safely build up a larger set of "recommended" compiler flags
>
> for
>
> each of the release types rather than just "-g" and "-O3".
>
> I think that -Wall should be the default flag, and that developers have
>
> to
>
> explicitly turn it off.  That way it's not just the 4 of us triaging
>
> all
>
> these complier warnings.
>
> NOTE:  The code below is not working, but I think it gets the point
>
> across.
>
> Hans
>
>
>
>
> #To create a portable build system, it is best to not test for
>
> platforms,
>
> but to test for features.
>
> #
>
> #Instead of testing "if Windows then do this", test for "if the
>
> -Wno-invalid-offsetof flag works then use it". You can do that with the
>
> CheckCCompilerFlag module, for example:
>
> include(CheckCCompilerFlag)
>
> include(CheckCXXCompilerFlag)
>
> set(C_RELEASE_DESIRED_FLAGS "")
>
> set(CXX_RELEASE_DESIRED_FLAGS "")
>
> # -Wall
>
> check_c_compiler_flag(-Wall HAS_ALL_WARNINGS_C)
>
> if (HAS_ALL_WARNINGS_C)
>
>    set(CMAKE_C_FLAGS  "${CMAKE_C_FLAGS} -Wall")
>
> endif()
>
> check_cxx_compiler_flag(-Wall HAS_ALL_WARNINGS_CXX)
>
> if (HAS_ALL_WARNINGS_CXX)
>
>    set(CMAKE_CXX_FLAGS  "${CMAKE_C_FLAGS} -Wall")
>
> endif()
>
> ## -pedantic
>
> check_c_compiler_flag(-pedantic HAS_PEDANTIC_C)
>
> if (HAS_PEDANTIC_C)
>
>    set(CMAKE_C_FLAGS  "${CMAKE_C_FLAGS} -pedantic")
>
> endif()
>
> check_cxx_compiler_flag(-pedantic HAS_PEDANTIC_CXX)
>
> if (HAS_PEDANTIC_CXX)
>
>    set(CMAKE_CXX_FLAGS  "${CMAKE_C_FLAGS} -pedantic")
>
> endif()
>
> ## -Wextra
>
> check_c_compiler_flag(-Wextra HAS_EXTRA_WARNINGS_C)
>
> if (HAS_EXTRA_WARNINGS_C)
>
>    set(CMAKE_C_FLAGS  "${CMAKE_C_FLAGS} -Wextra")
>
> endif()
>
> check_cxx_compiler_flag(-Wextra HAS_EXTRA_WARNINGS_CXX)
>
> if (HAS_EXTRA_WARNINGS_CXX)
>
>    set(CMAKE_CXX_FLAGS  "${CMAKE_C_FLAGS} -Wextra")
>
> endif()
>
> ## -Wshadow
>
> check_c_compiler_flag(-Wshadow HAS_SHADOW_WARNINGS_C)
>
> if (HAS_SHADOW_WARNINGS_C)
>
>    set(CMAKE_C_FLAGS  "${CMAKE_C_FLAGS} -Wshadow")
>
> endif()
>
> check_cxx_compiler_flag(-Wshadow HAS_SHADOW_WARNINGS_CXX)
>
> if (HAS_SHADOW_WARNINGS_CXX)
>
>    set(CMAKE_CXX_FLAGS "${CMAKE_C_FLAGS} -Wshadow")
>
> endif()
>
> ## -Wlong-long  We should not use the long-long type in ITK
>
> check_c_compiler_flag(-Wlong-long HAS_LONG_LONG_WARNINGS_C)
>
> if (HAS_LONG_LONG_WARNINGS_C)
>
>    set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wlong-long")
>
> endif()
>
> check_cxx_compiler_flag(-Wlong-long HAS_LONG_LONG_WARNINGS_CXX)
>
> if (HAS_LONG_LONG_WARNINGS_CXX)
>
>    set(CMAKE_CXX_FLAGS "${CMAKE_C_FLAGS} -Wlong-long")
>
> endif()
>
>
> check_cxx_compiler_flag(-Wno-invalid-offsetof HAS_NO_INVALID_OFFSETOF)
>
> if (HAS_NO_INVALID_OFFSETOF)
>
>    set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wno-invalid-offsetof")
>
> endif()
>
> message(${CMAKE_C_FLAGS})
>
> message(${CMAKE_CXX_FLAGS})
>
>
>
> On 1/6/11 2:55 PM, "Luis Ibanez" <luis.ibanez at kitware.com> wrote:
>
> I just added -Wall and -Wextra to the builds in
>
> zion, thurmite and eldorado.
>
> It's time to follow Bill L. in the zero-warning campaign      :-)
>
>
>      Luis
>
>
> ----------------------------------
>
> On Wed, Jan 5, 2011 at 6:32 PM, Sean McBride <sean at rogue-research.com>
>
> wrote:
>
> That build is only using -Wall and -Wextra, which my gcc dashboards
>
> also
>
> use, and yet mine show a much smaller number of warnings.  Strange
>
> that
>
> a newer gcc would not warn where an older gcc does, one reason could
>
> be
>
> that they were false positives in the older version.  In any case,
>
> there
>
> are less warnings than it seems since some are caused by headers and
>
> so
>
> repeated in many source files.
>
> On Wed, 5 Jan 2011 17:01:46 -0500, Bill Lorensen said:
>
> I think they are valid warnings.
>
> On Wed, Jan 5, 2011 at 4:20 PM, Hans Johnson
>
> <hans-johnson at uiowa.edu> wrote:
>
> This looks like the Insight-gcc-3.3. Has a definitive decision been
>
> made
>
> regarding compilers less than 4?  I thought that less than 3.4 was
>
> excluded
>
> because of incompatibility issues.
>
>
>
>
> --
>
> Hans J. Johnson, Ph.D.
>
> Assistant Professor
>
> 200 Hawkins Drive
>
> T205 BT, The University of Iowa
>
> Iowa City, IA 52242
>
> hans-johnson at uiowa.edu
>
> PHONE: 319 353 8587
>
> _______________________________________________
>
> Powered by www.kitware.com
>
> Visit other Kitware open-source projects at
>
> http://www.kitware.com/opensource/opensource.html
>
> Kitware offers ITK Training Courses, for more information visit:
>
> http://kitware.com/products/protraining.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
>
> _______________________________________________
>
> Powered by www.kitware.com
>
> Visit other Kitware open-source projects at
>
> http://www.kitware.com/opensource/opensource.html
>
> Kitware offers ITK Training Courses, for more information visit:
>
> http://kitware.com/products/protraining.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
>
>
> <ATT00001..txt>
>
>
> ========================================================
>
> Bradley Lowekamp
>
> Lockheed Martin Contractor for
>
> Office of High Performance Computing and Communications
>
> National Library of Medicine
>
> blowekamp at mail.nih.gov
>
>