Re: [Lcms-user] Build warnings on OSX / clang
Thanks for reporting. lcms uses the C feature of implicit sign conversion, so if you activate -Wsign-conversion you will get some warnings. This not set in the included build system. Regards Marti On 7/24/2017 3:16 AM, Aaron Boxer wrote: Hello, Just wanted to report a few warnings I am getting when building with clang. http://my.cdash.org/viewBuildError.php?type=1&buildid=1257347 For example: grok/thirdparty/liblcms2/src/cmsgamma.c:143:32: warning: implicit conversion changes signedness: 'cmsUInt32Number' (aka 'unsigned int') to 'int' [-Wsign-conversion] fl ->nFunctions = Plugin ->nFunctions; ~ ~^~ grok/thirdparty/liblcms2/src/cmsgamma.c:150:63: warning: implicit conversion changes signedness: 'int' to 'unsigned long' [-Wsign-conversion] memmove(fl->FunctionTypes, Plugin ->FunctionTypes, fl->nFunctions * sizeof(cmsUInt32Number)); Thanks, Aaron -- 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 Lcms-user@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/lcms-user -- 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 Lcms-user@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/lcms-user
Re: [Lcms-user] Build warnings on OSX / clang
Hi Marti, Thanks. I've turned on this warning in my build because the implicit conversion can cause hard-to-trace bugs. Regards, Aaron On Mon, Jul 24, 2017 at 3:10 AM, Martí Maria wrote: > > Thanks for reporting. > > lcms uses the C feature of implicit sign conversion, so if you activate > -Wsign-conversion you will get some warnings. > > This not set in the included build system. > > Regards > > Marti > > On 7/24/2017 3:16 AM, Aaron Boxer wrote: > > Hello, > Just wanted to report a few warnings I am getting when building with clang. > > http://my.cdash.org/viewBuildError.php?type=1&buildid=1257347 > > For example: > > href='https://github.com/GrokImageCompression/grok/blob/master/grok/thirdparty/liblcms2/src/cmsgamma.c#L143'>grok/thirdparty/liblcms2/src/cmsgamma.c:143:32: > warning: implicit conversion changes signedness: 'cmsUInt32Number' (aka > 'unsigned int') to 'int' [-Wsign-conversion] > fl ->nFunctions = Plugin ->nFunctions; > ~ ~^~ > href='https://github.com/GrokImageCompression/grok/blob/master/grok/thirdparty/liblcms2/src/cmsgamma.c#L150'>grok/thirdparty/liblcms2/src/cmsgamma.c:150:63: > warning: implicit conversion changes signedness: 'int' to 'unsigned long' > [-Wsign-conversion] > memmove(fl->FunctionTypes, Plugin ->FunctionTypes, fl->nFunctions * > sizeof(cmsUInt32Number)); > > > Thanks, > > Aaron > > > > -- > 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 > listLcms-user@lists.sourceforge.nethttps://lists.sourceforge.net/lists/listinfo/lcms-user > > > > > -- > 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 > Lcms-user@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/lcms-user > > -- 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 Lcms-user@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/lcms-user
Re: [Lcms-user] Build warnings on OSX / clang
On Mon, 24 Jul 2017, Aaron Boxer wrote: Hi Marti, Thanks. I've turned on this warning in my build because the implicit conversion can cause hard-to-trace bugs. Inappropriate explicit casts in the C code can also cause hard-to-trace bugs and explicit casts should be minimized. I think that usually implicit type conversion should be preferred over many explicit casts. Regardless, type conversion should be minimized as much as possible. Not all compiler warnings are intended to be enabled for normal use. Bob -- Bob Friesenhahn bfrie...@simple.dallas.tx.us, http://www.simplesystems.org/users/bfriesen/ GraphicsMagick Maintainer,http://www.GraphicsMagick.org/ -- 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 Lcms-user@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/lcms-user
Re: [Lcms-user] Build warnings on OSX / clang
> Thanks. I've turned on this warning in my build because the implicit >> conversion can cause >> hard-to-trace bugs. >> > > Inappropriate explicit casts in the C code can also cause hard-to-trace > bugs and explicit casts should be minimized. I think that usually implicit > type conversion should be preferred over many explicit casts. Regardless, > type conversion should be minimized as much as possible. > Absolutely agree - casts are a necessary evil :) > > Not all compiler warnings are intended to be enabled for normal use. > > Yes, I suppose I could disable the warning for production. Aaron -- 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 Lcms-user@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/lcms-user
Re: [Lcms-user] Build warnings on OSX / clang
Hello all, Compiler warnings are available to show us where programmers have taken liberties with the language, but have not acknowledged those liberties (via a cast). IMO, the C language implicit integer assignment rules were a shortcoming of the language, leading to sloppy work, not a feature. In our projects all our code builds clean with Visual Studio's max warning level (-Wall) and Code Analysis. We don't allow warnings to persist, and I honestly don't know why - other than economics - anyone would want to set a project up to use anything less. I don't mean to be critical of Marti's code - it's very good overall and emits no warnings with Visual Studio 2017 on level 4 (which is better than most source projects), but it DOES gather hundreds of warnings when set to -Wall, many of which are of the form: ..\..\..\src\cmsgamma.c(143): warning C4365: '=': conversion from 'cmsUInt32Number' to 'int', signed/unsigned mismatch Unfortunately, it really IS a coding error to expect an unsigned integer to fit implicitly into a signed integer. If you "just know" that there can be no sign problems, then the code should reflect that. The right solution is not to add casts, but rather to change the code so that the data types DO match one another - or in a pinch use explicit casts in the code (with appropriate comments) indicating that the disparity has been noted and is expected and okay. In the specific case identified (an unsigned 32 bit number being copied to a signed 32 bit value) we can't know, without careful analysis of the code, whether there can be (now, or in the future) a negative number or positive 32 bit number larger than 2^31 passed around. That's a gotcha that should not be left in. If types have to change, of course it's necessary to work out the problems each code change could cause. That's the cost of creating truly good code. The upside is that when it's done the code can be consistently compiled with the highest warning level, which helps keep errors from creeping in during ongoing development. Marti, I'm willing to take a look at reducing the warning count if you're willing to entertain putting the changes back into your sources (as you have done in the past). -Noel Carboni ProDigital Software -- 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 Lcms-user@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/lcms-user
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.RegardsMarti-- 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 Lcms-user@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/lcms-user
Re: [Lcms-user] Build warnings on OSX / clang
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:marti.ma...@littlecms.com] Sent: Mon, July 24, 2017 6:09 PM To: Noel Carboni Cc: lcms-user@lists.sourceforge.net 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 Lcms-user@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/lcms-user
Re: [Lcms-user] Build warnings on OSX / clang
Sounds great. All improvements are more than welcome.RegardsMartiOn 25 Jul 2017 00:27, Noel Carboni 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:marti.maria@littlecms.com] Sent: Mon, July 24, 2017 6:09 PMTo: Noel CarboniCc: lcms-user@lists.sourceforge.netSubject: 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. RegardsMarti-- 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 Lcms-user@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/lcms-user
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 wrote: > Sounds great. All improvements are more than welcome. > Regards > Marti > > On 25 Jul 2017 00:27, Noel Carboni > 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:marti.ma...@littlecms.com] > *Sent:* Mon, July 24, 2017 6:09 PM > *To:* Noel Carboni > *Cc:* lcms-user@lists.sourceforge.net > *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 > Lcms-user@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/lcms-user > > -- 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 Lcms-user@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/lcms-user
Re: [Lcms-user] Build warnings on OSX / clang
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 'x' selected for automatic inline expansion warning C4820: '': 'n' bytes padding added after data member warning C4061: enumerator 'x' in switch of enum 'y' 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:boxe...@gmail.com] Sent: Mon, July 24, 2017 7:03 PM To: Marti Maria Cc: Noel Carboni; lcms-user@lists.sourceforge.net 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 wrote: Sounds great. All improvements are more than welcome. Regards Marti On 25 Jul 2017 00:27, Noel Carboni 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" fro
Re: [Lcms-user] Build warnings on OSX / clang
On 26 July 2017 at 05:26, Noel Carboni wrote: > Here's the source code: > http://Noel.ProDigitalSoftware.com/temp/LittleCMS_2.8NC.zip Can you provide a patch please? With a patch I can test it with the colord self tests which run on quite a few different types of machine. Thanks. Richard. -- 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 Lcms-user@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/lcms-user
Re: [Lcms-user] Build warnings on OSX / clang
Hi Noel, Many thanks for your hard work. There are, however, two important issues: - You modified sources other than latest ones in GIT. That means all changes since 2.8 are lost. Changes are big, and this means merging is difficult and error prone. - The testbed reports multiple failures in critical parts like interpolation and math in general. This is bad :( I would be glad to accept some changes to silence compiler warnings, but I'm afraid merging those all changes and make sure all is working would take me months that currently I don't have. Regards Marti. On 7/26/2017 6:26 AM, Noel Carboni 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 'x' selected for automatic inline expansion warning C4820: '': 'n' bytes padding added after data member warning C4061: enumerator 'x' in switch of enum 'y' 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:boxe...@gmail.com] *Sent:* Mon, July 24, 2017 7:03 PM *To:* Marti Maria *Cc:* Noel Carboni; lcms-user@lists.sourceforge.net *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 mailto:marti.ma...@littlecms.com>> wrote: Sounds great. All improvements are more than welcome. Regards Marti On 25 Jul 2017 00:27, Noel Carboni <mailto:ncarb...@prodigitalsoftware.com>> wrote:
Re: [Lcms-user] Build warnings on OSX / clang
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 < ncarb...@prodigitalsoftware.com> 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 'x' selected for > automatic inline expansion > > warning C4820: '': 'n' bytes padding added after data > member > > warning C4061: enumerator 'x' in switch of enum > 'y' 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:boxe...@gmail.com] > *Sent:* Mon, July 24, 2017 7:03 PM > *To:* Marti Maria > *Cc:* Noel Carboni; lcms-user@lists.sourceforge.net > > *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 i
Re: [Lcms-user] Build warnings on OSX / clang
Hi Marti, Thanks for the feedback on seeing errors for interpolation and math; I'm sorry I broke something. I was afraid that might happen, since I wasn't clear in a couple of places on whether the math was intended to actually able to handle negative numbers. I already have a pretty good idea of where the problems may be. There is also a small chance I corrected some inaccuracies, but at this point I trust the test bed more than my own recent work. I'll be happy to remake the changes against your latest sources from Git, assuming I can find them, since my changes are fresh in my mind and they really didn't take long to do. And I do need to take the time to enable the testbed and verify my work myself. My intent is certainly not to make additional work for you so that everyone can gain the potential benefits of more rigorous compiler checking. So everyone else on the mailing list: Please at this point hold off taking the sources I posted before, at least until I can at least pass them through the testbed tests successfully or know exactly why they fail. -Noel From: Martí Maria [mailto:marti.ma...@littlecms.com] Sent: Wed, July 26, 2017 3:50 AM To: Noel Carboni; lcms-user@lists.sourceforge.net Cc: 'Aaron Boxer' Subject: Re: [Lcms-user] Build warnings on OSX / clang Hi Noel, Many thanks for your hard work. There are, however, two important issues: - You modified sources other than latest ones in GIT. That means all changes since 2.8 are lost. Changes are big, and this means merging is difficult and error prone. - The testbed reports multiple failures in critical parts like interpolation and math in general. This is bad :( I would be glad to accept some changes to silence compiler warnings, but I'm afraid merging those all changes and make sure all is working would take me months that currently I don't have. Regards Marti. On 7/26/2017 6:26 AM, Noel Carboni 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 'x' selected for automatic inline expansion warning C4820: '': 'n' bytes padding added after data member warning C4061: enumerator 'x' in switch of enum 'y' 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 te
Re: [Lcms-user] Build warnings on OSX / clang
On Wed, 26 Jul 2017, Aaron Boxer wrote: 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 ? The normal way (quite well documented) is ./configure CFLAGS='-O2 -Wall' ... I use these GCC options while building GraphicsMagick: ./configure 'CFLAGS=-O2 -g -ggdb -Wall -Winline -W -Wformat-security\ -Wpointer-arith -Wdisabled-optimization -Wdeclaration-after-statement' There are of course many more warning options which can be enabled for people who have plenty of time on their hands. Even these options are not likely to include type conversion warnings. Optimizing warnings for just one compiler is a bad idea. At least three completely different compilers should be used. Bob -- Bob Friesenhahn bfrie...@simple.dallas.tx.us, http://www.simplesystems.org/users/bfriesen/ GraphicsMagick Maintainer,http://www.GraphicsMagick.org/ -- 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 Lcms-user@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/lcms-user
Re: [Lcms-user] Build warnings on OSX / clang
> 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 ? There is a capability to do the checking on Windows as well; I just don't have it set up presently. We rushed through getting the project to build with VS 2017 when it came out and never brought up the test bed stuff. As I mentioned before, everyone wait w/regard to trying to use the sources I posted earlier, as I need to both figure out what I broke as well as merge the changes into the current trunk to be of the most help to Marti. -Noel -- 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 Lcms-user@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/lcms-user
Re: [Lcms-user] Build warnings on OSX / clang
That is what I get when cross-compiling to ARM by gcc 4.6.3 arm-elf-gcc.exe -c -DCMS_NO_PTHREADS -std=c99 --pedantic -Wall -I ../include *.c cmscgats.c: In function 'ParseFloatNumber': cmscgats.c:643:5: warning: array subscript has type 'char' [-Wchar-subscripts] cmsopt.c: In function 'OptimizeByComputingLinearization': cmsopt.c:1035:26: warning: variable 'lIsLinear' set but not used [-Wunused-but-set-variable] cmspcs.c: In function '_cmsLCMScolorSpace': cmspcs.c:872:5: warning: overflow in implicit constant conversion [-Woverflow] I tried also a modern compiler. gcc 6.3.1 complains on indentation because -Wmisleading-indentation, so turning it off: arm-none-eabi-gcc.exe -c -DCMS_NO_PTHREADS -std=c99 --pedantic -Wall -Wno-misleading-indentation -I ../include *.c cmsopt.c: In function 'OptimizeByComputingLinearization': cmsopt.c:1035:26: warning: variable 'lIsLinear' set but not used [-Wunused-but-set-variable] cmsBool lIsSuitable, lIsLinear; ^ cmspcs.c: In function '_cmsLCMScolorSpace': cmspcs.c:872:22: warning: overflow in implicit constant conversion [-Woverflow] default: return (cmsColorSpaceSignature) (-1); ^ Again, please note that this is using C language, not C++. Marti On 7/26/2017 3:37 PM, Bob Friesenhahn wrote: On Wed, 26 Jul 2017, Aaron Boxer wrote: 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 ? The normal way (quite well documented) is ./configure CFLAGS='-O2 -Wall' ... I use these GCC options while building GraphicsMagick: ./configure 'CFLAGS=-O2 -g -ggdb -Wall -Winline -W -Wformat-security\ -Wpointer-arith -Wdisabled-optimization -Wdeclaration-after-statement' There are of course many more warning options which can be enabled for people who have plenty of time on their hands. Even these options are not likely to include type conversion warnings. Optimizing warnings for just one compiler is a bad idea. At least three completely different compilers should be used. Bob -- 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 Lcms-user@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/lcms-user
Re: [Lcms-user] Build warnings on OSX / clang
> please note that this is using C language, not C++. It is possible the VS 2017 C++ is compiler is pickier about matching data types. There is nothing wrong with that, as it could uncover issues where data types are not consistent. LOL about your comment earlier about it taking months to merge the changes. I finished it in about 90 minutes. May I recommend the tool Beyond Compare by Scooter Software. :-) I'm now setting about doing the testing with the test suite. -Noel -- 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 Lcms-user@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/lcms-user
Re: [Lcms-user] Build warnings on OSX / clang
/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 'x' selected for automatic inline expansion warning C4820: '': 'n' bytes padding added after data member warning C4061: enumerator 'x' in switch of enum 'y' 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:boxe...@gmail.com] *Sent:* Mon, July 24, 2017 7:03 PM *To:* Marti Maria *Cc:* Noel Carboni; lcms-user@lists.sourceforge.net *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 mailto:marti.ma...@littlecms.com>> wrote: Sounds great. All improvements are more than welcome. Regards Marti On 25 Jul 2017 00:27, Noel Carboni <mailto:ncarb...@prodigitalsoftware.com>> 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:marti.ma...@littlecms.com <mailto:marti.ma...@littlecms.com>] *Sent:* Mon, July 24, 2017 6:09 PM *To:* Noel Carboni *Cc:* lcms-user@lists.sourceforge.net <mailto:lcms-user@lists.sourceforge.net> *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.