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:[email protected]]
*Sent:* Mon, July 24, 2017 7:03 PM
*To:* Marti Maria
*Cc:* Noel Carboni; [email protected]
*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
<[email protected] <mailto:[email protected]>> wrote:
Sounds great. All improvements are more than welcome.
Regards
Marti
On 25 Jul 2017 00:27, Noel Carboni <[email protected]
<mailto:[email protected]>> 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:[email protected]
<mailto:[email protected]>]
*Sent:* Mon, July 24, 2017 6:09 PM
*To:* Noel Carboni
*Cc:* [email protected]
<mailto:[email protected]>
*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
[email protected] <mailto:[email protected]>
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/lcms-user
