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

[Martí M. <mar...@li...>]

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 '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... <mailto:mar...@li...>> wrote:
>
> Sounds great. All improvements are more than welcome.
>
> Regards
>
> Marti
>
> On 25 Jul 2017 00:27, Noel Carboni <NCa...@Pr... 
> <mailto: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... 
> <mailto:mar...@li...>]
> *Sent:* Mon, July 24, 2017 6:09 PM
> *To:* Noel Carboni
> *Cc:* lcm...@li... 
> <mailto: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... <mailto:Lcm...@li...>
> https://lists.sourceforge.net/lists/listinfo/lcms-user
>