Re: [Lcms-user] Build warnings on OSX / clang

[Aaron B. <bo...@gm...>]

Thanks, Noel. Might be safer to do this on linux, where you can run make
check
to test. May I ask how I turn on -Wall on linux build for lcms ?

On Wed, Jul 26, 2017 at 12:26 AM, Noel Carboni <
NCa...@pr...> wrote:

> Hi guys,
>
>
>
> I've had a pass through the code.  I made several hundred changes to 20 or
> so source files, and now have it building here with warnings set to max in
> Visual Studio 2017.  Mostly the changes were to make unsigned variables out
> of signed variables, and I didn't have to add much casting.
>
>
>
> I have tested it in my own application (which of course only exercises
> part of the library), and it seems to work great.  I'm not presently set up
> to run the battery of tests included with the software, so I hope someone
> can help with that.  It would be also interesting to know if other
> compilers (e.g., clang) find fault where Visual Studio does not.
>
> Here's the source code:
>
>
>
> http://Noel.ProDigitalSoftware.com/temp/LittleCMS_2.8NC.zip
>
>
>
> I've made a number of changes to the release 2.8 sources with the
> following philosophies in mind:
>
>
>
> 1.  Don't alter the external interface (lcms2.h)
>
>
>
> 2.  Minimize the changes Marti will have to review and do them as safely
> as possible.
>
>
>
> 3.  Make the use of signed and unsigned types consistent to reduce the
> hundreds of warnings emitted when -Wall is used.
>
>
>
> 4.  Use as few casts as possible.
>
>
>
> 4.  Maintain efficiency.
>
>
>
>
>
> With this set of files you can now enable -Wall in Visual Studio 2017
> builds for ongoing LittleCMS development work, to gain the benefits of
> tighter type checking, but with the following caveats:
>
>
>
> A.  You'll most likely want to specifically disable several of the -Wall
> warnings by putting the following additional options on the C/C++ compiler
> command line:
>
>
>
>          /wd4711 /wd4820 /wd4061 /wd4774 /wd4710 /wd4668
>
>
>
>      These specifically quiet the following Visual Studio 2017 C++
> warnings, which may not be helpful to you:
>
>
>
>          warning C4711: function 'xxxxxxxxxxxxxxxxx' selected for
> automatic inline expansion
>
>          warning C4820: 'xxxxxxxx': 'n' bytes padding added after data
> member
>
>          warning C4061: enumerator 'xxxxxxxxxxxxx' in switch of enum
> 'yyyyyyyyyyyyy' is not explicitly handled by a case label
>
>          warning C4774: '_snprintf' : format string expected in argument 3
> is not a string literal
>
>          warning C4710: 'int _snprintf(char *const ,const ::size_t,const
> char *const ,...)': function not inlined
>
>          warning C4668: '_M_HYBRID_X86_ARM64' is not defined as a
> preprocessor macro, replacing with '0' for '#if/#elif'
>
>
>
> B.  There is one warning, in cmsxform.c, that may indicate a real problem,
> especially in light of compilation of this library for different systems
> with different compilers.  Marti, you may want to review the code and
> decide whether to change it or suppress the warning:
>
>
>
>          ..\..\..\src\cmsxform.c(799): warning C4191: 'type cast': unsafe
> conversion from '_cmsTransform2Fn' to '_cmsTransformFn'
>
>
>
> The above text, to help Marti see what I've done, is also in a comment
> block in lcms2_internal.h.
>
>
>
> I suggest careful review and testing of these changes should be done.  I
> don't think I have corrected any serious logic errors, and I hope I haven't
> caused any new ones.  The compiler's additional scrutiny can be brought to
> bear going forward to help Marti et. al. with ongoing development.
>
>
>
> -Noel
>
>
>
> *From:* Aaron Boxer [mailto:bo...@gm...]
> *Sent:* Mon, July 24, 2017 7:03 PM
> *To:* Marti Maria
> *Cc:* Noel Carboni; lcm...@li...
>
> *Subject:* Re: [Lcms-user] Build warnings on OSX / clang
>
>
>
> Thanks to Noel for offering fixes, and to Marti for graciously accepting :)
>
> I've gone through similar issues with my JPEG 2000 codec, much of the code
> inherited from developers
> who didn't worry about these issues.  I spent a lot of time converting
> signed to unsigned for all quantities
> that must always be positive : number of image components, image
> dimensions, etc.
>
> Also, removed as many casts as possible. The code is now more resilient to
> overflow, and compiles with no
>
> warnings at maximum -Wall compiler settings. I think it is worth the
> effort.
>
> Regards,
>
> Aaron
>
>
>
>
>
>
>
>
> On Mon, Jul 24, 2017 at 6:31 PM, Marti Maria <mar...@li...>
> wrote:
>
> Sounds great. All improvements are more than welcome.
>
> Regards
>
> Marti
>
>
>
> On 25 Jul 2017 00:27, Noel Carboni <NCa...@Pr...>
> wrote:
>
> Hi Marti,
>
>
>
> As C/C++ programmers we have to face the fact that the language is growing
> ever more tightly typed.
>
>
>
> It's not quite correct even to promote implicitly from signed to unsigned
> when you "just know" that the values will be small, since under some
> conditions negative numbers could have meaning.  Or worse, be given special
> meaning in the future.  It's tempting to return -1 as a special error or
> "not found" case, but that can ultimately cause conflict with code that
> expects only signed values.
>
>
>
> It's only really properly done - and thus "safe" from future maintenance
> problems - if you use consistent types across the board, or add casts
> occasionally when you really do want to express that you're fitting a small
> positive unsigned value into a signed field.
>
>
>
> Looking your released code over just now I think that it can be
> substantially cleaned up without a lot of casts added.  There are a lot of
> cases where you have a habit of using "int" when what you really want is
> "unsigned int" (e.g., for number of channels, where negative values don't
> have meaning).  :)
>
>
>
> Tell you what; I'll go through all the code and see what I can come up
> with to get it down to zero warnings at max warning level.  I don't think
> it'll take me more than a few man-hours.  I'll send the result to you and
> you can compare the files and see whether you feel the changes are safe
> enough.  I'll make it my mission not to change the logic or your intent
> with the style.
>
>
>
> -Noel
>
>
>
> *From:* Marti Maria [mailto:mar...@li...]
> *Sent:* Mon, July 24, 2017 6:09 PM
> *To:* Noel Carboni
> *Cc:* lcm...@li...
> *Subject:* Re: [Lcms-user] Build warnings on OSX / clang
>
>
>
> Hi,
>
>
>
> Sometimes I use  signed to unsigned promotion without cast. This is safe
>  if values are small and positive. Mostly to avoid casts in memset,
> memmove, calloc, etc. which looks really ugly.
>
>
>
> If you can found other cases, like that one in cmsgamma.c, please let me
> know. But this latter was fixed after the original report.
>
>
>
> Regards
>
> Marti
>
>
>
>
> ------------------------------------------------------------
> ------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> Lcms-user mailing list
> Lcm...@li...
> https://lists.sourceforge.net/lists/listinfo/lcms-user
>
>
>