Hi, I’ve been working on improving the CI coverage and adding additional warnings to improve the overall quality of the codebase, without changing any existing behaviour. Since there are a large number of changes, I just wanted to bring it up here for discussion in case any of this was problematic or there were better approaches to consider. Bob asked for some discussion of this in https://gitlab.com/libtiff/libtiff/-/merge_requests/800#note_2975630537 so this is the reason for the email.
Essentially, what I’ve been doing is incrementally adding additional compiler warning flags to the extra-warnings test_flags list in cmake/CompilerChecks.cmake and then fixing any warnings arising from the addition on all of the tested platforms and compilers. Example here: https://gitlab.com/libtiff/libtiff/-/pipelines/2235344591. This tests several GCC, clang and MSVC versions over multiple hardware platforms and operating systems. Overall the intention is to catch and fix bugs which had been hidden but unnoticed, and to change implicit behaviours to explicit so that it is clear from the code what is happening and why, so that the intent is obvious. I intend to add additional CI static analysis checking with e.g. clang-tidy once we have established this baseline known good state. Most of the additions are benign and did not require code changes, or required only minimal changes. However, some required extensive modifications, including: * -Wswitch-default and -Wswitch-enum: These required additions to many case statements * -Wconversion: Addition of many casts * -Wwrite-strings: Conversion of char* to const char* in multiple places * -Wsign-conversion: Conversion from unsigned to signed and vice versa One question here is whether the added code complexity and added maintenance burden is worth it? My take on this is hopefully not too extreme, but it’s essentially this: the overall quality of this old C codebase is not good, and I’d like to improve it by adding and enforcing a whole host of strict checks. This is to enforce a minimum level of quality, and to prevent regressions using the CI pipelines to aggressively test and fail code changes which don’t meet the quality bar. This does mean that a build which passes for a developer locally may fail in CI. It will pick up a whole host of {compiler, OS, platform, configuration}-specific special-cases which might otherwise not be noticed. And it will pick up on a lot of trivial mistakes that might not otherwise be noticed. This does increase the demands on each developer, but most of this should be automated by the CI infrastructure. Developers would need to use extra-warnings by default to catch most errors early. Most of the work is in bringing up the codebase to the required standard; keeping it that way once established should be far less work. Unfixed: * -Wcast-qual * -Wcast-align These two require more extensive work to properly correct, potentially involving refactoring to change some structures and some of the code to properly align things, and to avoid any removal of constness from any constant value. I haven’t looked at these for this piece of work; it will need investigating and scoping separately. But I think it would be worth doing for safety and correctness. Kind regards, Roger Branch summaries. I split this up over multiple branches to keep the changes reviewable in chunks. ---- warning-additions https://gitlab.com/libtiff/libtiff/-/merge_requests/798 (merged) Warning Flags Added: * -Wformat-nonliteral, -Wformat-signedness, -Wformat-truncation * Added extra-warnings and broken-warnings cmake options Code Changes: Format specifier fixes (-Wformat): * Changed %d to %u for unsigned values across many files (tif_dir.c, tif_dirinfo.c, tif_dirread.c, tif_lerc.c, tif_getimage.c, tif_fax3.h) * Changed %d to %u in iptcutil.c for dataset and recnum * Fixed TIFF_MAX_DIR_COUNT format to cast to (unsigned) * Fixed sp->line format from %u to %d (it's signed) Pedantic fixes: * Changed empty parameter lists () to (void) in function declarations (tiff-grayscale.c, tiff-palette.c, tiff-rgb.c) * Cast sp->predictor to (unsigned) for hex format specifier CI changes: * Pre-build changed to static * CMake builds now use extra-warnings ---- warning-additions2 https://gitlab.com/libtiff/libtiff/-/merge_requests/800 Warning Flags Added: * -Wnull-dereference * -Wshadow * -Wstrict-prototypes and -Wmissing-prototypes * -Wswitch-default * -Wswitch-enum Code Changes: Switch statement completeness (-Wswitch-enum, -Wswitch-default): * Added explicit case labels for all enum values in switch statements across: - tif_dir.c: Added TIFF_NOTYPE, TIFF_ASCII cases - tif_dirinfo.c: Added exhaustive TIFF_SETGET_* cases - tif_dirread.c: Added TIFFReadDirEntryErrOk case - tif_dirwrite.c: Added ~50 explicit TIFF_SETGET_* cases - tif_lzma.c: Added all lzma_ret enum values - tif_ojpeg.c: Added osibsEof, osibsNotSetYet, osibsJpegInterchangeFormat cases - tif_webp.c: Added VP8_ENC_OK, VP8_ENC_ERROR_LAST cases - Test files: Similar exhaustive case additions LZMA version guards: * Added #if LZMA_VERSION >= 50040000 guards for LZMA 5.4.0+ enums (LZMA_SEEK_NEEDED, LZMA_RET_INTERNAL*) Prototype declarations (-Wstrict-prototypes, -Wmissing-prototypes): * Added function prototypes to header files * Created new headers: test/check_tag.h, test/strip.h * Added static to file-local functions * Added function declarations to tools Shadow variable fixes (-Wshadow): * Renamed local variables that shadowed outer scope variables in test files and tools ---- warning-additions3 https://gitlab.com/libtiff/libtiff/-/merge_requests/802 Warning Flags Added: * -Wwrite-strings * -Wc99-c11-compat * -Wconversion Code Changes: Conversion fixes (-Wconversion): Large number of changes (1500+ line diff) adding explicit casts to prevent implicit conversions: * Integer arithmetic casts: Added (uint32_t), (size_t), (toff_t), (tmsize_t) casts throughout: - contrib/addtiffo/tif_overview.c: ~60 casts for pointer arithmetic and loop indices - contrib/addtiffo/tif_ovrcache.c: Casts for block size calculations * libtiff core: - tif_dirread.c, tif_dirwrite.c: Casts for tag value conversions - tif_fax3.c, tif_fax3.h: Casts for bit manipulation - tif_getimage.c: Casts for image dimension calculations - tif_jpeg.c, tif_lerc.c, tif_lzw.c, tif_webp.c, etc.: Codec-specific casts - tif_read.c, tif_write.c: I/O size casts * Tools: Extensive casts in all tools (tiff2pdf.c, tiffcrop.c, tiff2ps.c, tiffcmp.c, etc.) * Tests: Similar conversion casts throughout test files String literal changes (-Wwrite-strings): * Changed char* to const char* for string literal assignments ---- warning-additions4 https://gitlab.com/libtiff/libtiff/-/merge_requests/804 Warning Flags Added: * -Wsign-conversion, -Warith-conversion, -Wdouble-promotion, -Wfloat-conversion, -Wfloat-equal * -Wuninitialized, -Wduplicated-branches, -Wduplicated-cond * -Wunused-parameter, -Wmissing-declarations, -Wredundant-decls * -Wsizeof-array-div, -Wsizeof-pointer-div, -Wsizeof-pointer-memaccess * -Wlogical-op, -Wlogical-not-parentheses * -Wno-int-to-pointer-cast, -Wdangling-else * -Wunreachable-code, -Wbool-operation * -Wmissing-include-dirs, -Wunused-local-typedefs, -Wmisleading-indentation, -Wunused-macros Code Changes: Sign conversion fixes (-Wsign-conversion): * Added explicit casts between signed and unsigned types: - (toff_t)(dircount * 12) and (toff_t)(dircount * 20) in tif_dirread.c - (uint32_t), (int), (size_t) casts throughout Unused parameter fixes (-Wunused-parameter): * Added (void)param; statements or TIFF_UNUSED(param) macros Float comparison fixes (-Wfloat-equal): * Changed direct float comparisons to use epsilon-based comparisons Double promotion fixes (-Wdouble-promotion): * Added explicit (double) casts or used f suffix for float literals Redundant declaration fixes: * Removed duplicate declarations * Consolidated declarations in headers Logic/control flow: * Added braces to clarify dangling else * Fixed misleading indentation ---- warning-additions5 https://gitlab.com/libtiff/libtiff/-/merge_requests/805 Warning Flags Added: * -Wundef * -Wold-style-definition * -Wnested-externs * -Wvla, -Wjump-misses-init * -Warray-bounds=3 (upgraded from level 2) * -Wstringop-overflow=4 * -Walloc-zero * -Wtrampolines * Extra MSVC warnings (C4100, C4189, C4244, C4267, etc.) Code Changes: Macro definition fixes (-Wundef): * Changed #ifdef MACRO to #if MACRO for macros that are always defined (0 or 1): - WORDS_BIGENDIAN: Changed from #ifdef to #if throughout (tif_open.c, tif_lzw.c, bmp2tiff.c, ras2tiff.c) - JPEG_LIB_MK1_OR_12BIT: Changed to #if defined() form * Changed #if HAVE_* to #ifdef HAVE_* for optional feature macros: - HAVE_FCNTL_H, HAVE_SYS_TYPES_H, HAVE_IO_H, HAVE_UNISTD_H, HAVE_GETOPT, HAVE_IEEEFP CMake modernization: * ProcessorChecks.cmake: Modernised endianness checks using CMAKE_C_BYTE_ORDER * WORDS_BIGENDIAN now always defined to 1 or 0 (not just defined/undefined) * Unset /W3 before setting /W4 on MSVC to avoid warning D9025 * Added system header suppression for Windows headers (/external:anglebrackets /external:W0) Consistency fixes: * tif_jpeg.c: Use HAVE_DECL_OPTARG and JPEG_LIB_MK1_OR_12BIT consistently * port/libport.h: Use #ifdef for HAVE_GETOPT and HAVE_UNISTD_H * Tools: Consistent use of HAVE_UNISTD_H Added broken-warnings category: * Flags that warn erroneously: -Wcast-qual, -Wcast-align, -Wpadded, -Wunsuffixed-float-constants, -Wstringop-truncation ---- Summary Statistics | Branch | New Flags | Files Changed | Lines Changed | |--------------------|-----------|---------------|---------------| | warning-additions | ~3 | 53 | +335/-246 | | warning-additions2 | 6 | 61 | +780/-121 | | warning-additions3 | 3 | 80 | +1542/-1504 | | warning-additions4 | 24 | 51 | +721/-762 | | warning-additions5 | 9 | 15 | +86/-52 | _______________________________________________ Tiff mailing list [email protected] https://lists.osgeo.org/mailman/listinfo/tiff
