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

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

Hello,


Following Noel Carboni's suggestion and proof of concept, I have 
submitted a big commit that  gets rid of signed/unsigned mixing warnings 
when compiling as C++ with -Wall -Wpedantic

I've checked it with several compilers, clang, Visual Studio 15 and gcc 
3, 4 and 5, Borland C 5.5 and Embarcadero C++ 10.1. All passes the 
testbed and all my internal test beds on plug-ins.

Some thoughts:

- There are changes in the API. Most are identifying what 'int' means 
really. I regard this as bug fixes, since size of int may change. Ditto 
for some parameters that had to be declared as unsigned and were not. 
Overall, no client app should be broken in any case.
- The interpolation routines are untouched, rather than putting tons of 
casts I used #pragma to prevent warnings. The default interpolation is 
fine-tuned to match Photoshop, this is very valuable for some users and 
changing it is extremely risky. For things like throughput gains, please 
see below.
- In some places there are now ugly casts that bring nothing but noise, 
but I understand in many other places a strict type check is very 
desirable, so here it is.

Now regarding throughput optimization.

 From time to time people keeps sending me improvements for increasing 
throughput. Many times that involves changing the default interpolation 
code or adding SSE or VEC assembly in the base lcms2. I appreciate those 
collaborations, but the default interpolation is matching quite well 
photoshop and there is a clean, documented ways to increase throughput 
without sacrificing compatibility. That is the plug-in system. There are 
several plug-in types devoted to throughput for different usages. The 
right function to call when using those plug-in is 
cmsDoTransformLineStride(), which prepares buffers in the proper way.

Just as an example, I have plug-ins for fast processing that delivers:

P E R F O R M A N C E   T E S T S   8  B I T S
==============================================

                                   MPixel/sec. MByte/sec.
8 bits on CLUT profiles         : 28.42        85.26
8 bits on Matrix-Shaper profiles: 85.56        256.68
8 bits on same Matrix-Shaper    : 432.43       1297.30
8 bits on curves                : 253.97       761.90

P E R F O R M A N C E   T E S T S  F L O A T  (P L U G I N)
===========================================================

                                   MPixel/sec. MByte/sec.
Floating point on CLUT profiles  : 20.13 241.51       (x 12.3)
Floating point on Matrix-Shaper  : 19.05 228.57       (x 7.8)
Floating point on same MatrixSh  : 106.67 1280.00      (x 53.9)
Floating point on curves         : 44.82 537.82       (x 20.5)

And others for other types with similar results. Sorry, those plugins 
are not released as open source and I want not to spam you with 
commercial ads. It is just and example that what can be done without 
touching the default routines.


You can reach latest changes at 
https://github.com/mm2/Little-CMS/commit/55df275dc58fc817a59bf52c1b2bb564433c3999 
or just by downloading the github lcms2 trunk

I will be packing lcms2.9 in few days. This version is supposed to add 
stability and will remain for a while as because personal matters I have 
actually little time to dedicate to lcms.

Thanks again, Noel for all your hard work in this issue.
Best 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
>