Fixes to various clang warnings, increased minimum CMake version #126

seanm · 2024-11-20T00:20:44Z

No description provided.

Swapped the two halves of the && check. Safer to check the index first, before accessing the buffer. Out of range access was probably possible the old way.

Several declarations and definitions didn't quite match.

This was an antique K&R style function declaration.

Just marked private functions as static.

… corresponding header

Unlike C++, the void is required here in C, strictly speaking.

These were unused.

Many of these header have been standardized since C89, others have been obsolete forever. This started as fixing some -Wundef warnings, but then I thought I'd clean up more.

Indeed this was an infinite recursion. Looking at the functions above, and following that pattern, I think this meant to call the variant without the `2`.

These variables were indeed unused, the sting not appearing elsewhere in the file.

PrintVersion() called exit() meaning it never returned. There is only caller of PrintVersion(), so just moved the exit() call from inside to outside the function.

The prototype in the header was already in an #if 0.

Fixes warning "Compatibility with CMake < 3.10 will be removed from a future version of CMake." from CMake 3.31 Also moved it higher to fix "cmake_minimum_required() should be called prior to this top-level project() call" warning.

seanm · 2024-11-20T00:21:40Z

@vfonov apologies for so many commits, but each is small. I thought it best to do 1 comment per warning flag.

Please review carefully. I'm no expert in this codebase!

gdevenyi · 2024-11-22T20:26:18Z

Looks good to me. @seanm does this all pass make test?

An expanded testing swapping this patched version in the minc-toolkit-v2 superbuild tests would also be welcome.

seanm · 2024-11-22T20:36:30Z

Looks good to me. @seanm does this all pass make test?

No, but neither does it before this PR. For me, the same tests fail before and after.

seanm · 2024-11-22T21:14:50Z

Looks good to me. @seanm does this all pass make test?

The following tests FAILED:
	 28 - verify_xfm_1 (Failed)
	 29 - verify_xfm_float (Failed)
	 30 - verify_xfm_short (Failed)
	 31 - verify_xfm_byte (Failed)
	 32 - verify_xfm_2 (Failed)
	 53 - minc2-leak-test (Failed)

Is this the case for everyone?

vfonov · 2024-11-23T03:33:28Z

Is this the case for everyone?

100% tests passed, 0 tests failed out of 54 on linux

seanm · 2024-11-24T15:48:03Z

Running one of the tests has a lot of HDF5 messages, I wonder if this isn't a linux vs macOS difference, but rather a due to different HDF5 versions. What version are you using? I'm using from git master.

vfonov · 2024-11-25T14:40:18Z

I just tried on 1.14.3 on linux, all tests passed.

seanm · 2024-11-25T15:08:08Z

OK I'll try the same version...

seanm · 2024-11-25T19:32:04Z

    Start 28: verify_xfm_1

28: Test command: /Users/sean/external/libminc-bin/testdir/verify_xfm "/Users/sean/external/libminc/testdir/vio_xfm_test/t3.xfm" "/Users/sean/external/libminc/testdir/vio_xfm_test/random2" "1e-6"
28: Working Directory: /Users/sean/external/libminc-bin/testdir
28: Test timeout computed to be: 1500
28: HDF5-DIAG: Error detected in HDF5 (1.14.3) thread 0:
28:   #000: /Users/sean/external/hdf5-1.14.3/src/H5D.c line 1061 in H5Dread(): can't synchronously read data
28:     major: Dataset
28:     minor: Read failed
28:   #001: /Users/sean/external/hdf5-1.14.3/src/H5D.c line 1008 in H5D__read_api_common(): can't read data
28:     major: Dataset
28:     minor: Read failed
28:   #002: /Users/sean/external/hdf5-1.14.3/src/H5VLcallback.c line 2092 in H5VL_dataset_read_direct(): dataset read failed
28:     major: Virtual Object Layer
28:     minor: Read failed
28:   #003: /Users/sean/external/hdf5-1.14.3/src/H5VLcallback.c line 2048 in H5VL__dataset_read(): dataset read failed
28:     major: Virtual Object Layer
28:     minor: Read failed
28:   #004: /Users/sean/external/hdf5-1.14.3/src/H5VLnative_dataset.c line 373 in H5VL__native_dataset_read(): can't read data
28:     major: Dataset
28:     minor: Read failed
28:   #005: /Users/sean/external/hdf5-1.14.3/src/H5Dio.c line 401 in H5D__read(): can't read data
28:     major: Dataset
28:     minor: Read failed
28:   #006: /Users/sean/external/hdf5-1.14.3/src/H5Dchunk.c line 2919 in H5D__chunk_read(): unable to read raw data chunk
28:     major: Low-level I/O
28:     minor: Read failed
28:   #007: /Users/sean/external/hdf5-1.14.3/src/H5Dchunk.c line 4548 in H5D__chunk_lock(): data pipeline read failed
28:     major: Dataset
28:     minor: Filter operation failed
28:   #008: /Users/sean/external/hdf5-1.14.3/src/H5Z.c line 1355 in H5Z_pipeline(): required filter 'deflate' is not registered
28:     major: Data filters
28:     minor: Read failed
28:   #009: /Users/sean/external/hdf5-1.14.3/src/H5PLint.c line 267 in H5PL_load(): can't find plugin. Check either HDF5_VOL_CONNECTOR, HDF5_PLUGIN_PATH, default location, or path set by H5PLxxx functions
28:     major: Plugin for dynamically loaded library
28:     minor: Object not found
28:   #010: /Users/sean/external/hdf5-1.14.3/src/H5PLpath.c line 804 in H5PL__find_plugin_in_path_table(): search in path /usr/local/hdf5/lib/plugin encountered an error
28:     major: Plugin for dynamically loaded library
28:     minor: Can't get value
28:   #011: /Users/sean/external/hdf5-1.14.3/src/H5PLpath.c line 857 in H5PL__find_plugin_in_path(): can't open directory (/usr/local/hdf5/lib/plugin). Please verify its existence
28:     major: Plugin for dynamically loaded library
28:     minor: Can't open directory or file
28: /Users/sean/external/libminc/libsrc/hdf_convenience.c:1557 (from miicv_get): Can't read dataset /minc-2.0/image/0/image
28: /Users/sean/external/libminc/volume_io/Prog_utils/print.c:226 (from miicv_free): volume_io error: input_transform: error reading transform.
28:
28: Failed to load transform '/Users/sean/external/libminc/testdir/vio_xfm_test/t3.xfm'
1/1 Test #28: verify_xfm_1 .....................***Failed    0.01 sec

Is it maybe just that I'm supposed to have a minc-2.0/image/0/image file somewhere?

gdevenyi · 2024-11-26T15:41:17Z

Is it maybe just that I'm supposed to have a minc-2.0/image/0/image file somewhere?

No minc-2.0/image/0/image is part of the MINC namespace in a MINC file.

The errors look like your HDF5 is installed in /Users/sean/external/hdf5-1.14.3 but it looks like its trying to load its plugins from /usr/local/hdf5/lib/plugin and can't find deflate which is used for the internal compression inside MINC files.

vfonov · 2024-11-26T23:13:59Z

So, I merged most of the commits into develop. The only one that is problematic is 9849b33 - those checks still needed on Linux, in particular because there is a difference between Apple and Linux there.

dzenanz · 2024-11-27T15:02:25Z

Then this PR should be formally closed?

vfonov · 2024-11-27T18:49:00Z

Then this PR should be formally closed?

probably, if we ignore one unaccepted commit.

seanm · 2024-11-27T19:57:14Z

can't find deflate which is used for the internal compression inside MINC files.

Ah, so this might be because configuring HDF5 was not finding my zlib, so I built without zlib support. HDF5 has a variable ZLIB_DIR with the description "ZLIB_DIR: The directory containing a CMake configuration file for ZLIB." which is annoying because macOS ships with zlib libraries and headers, but not a cmake config file. It's strange that HDF5, unlike say vtk and itk and other libraries, doesn't seem to have variables to just point to the zlib library file and headers folder. I'll take this up with them...

The only one that is problematic is 9849b33 - those checks still needed on Linux

Do you recall which?

probably, if we ignore one unaccepted commit.

Sure, we can close this now.

dzenanz · 2024-11-27T20:07:43Z

I cannot close this. Either PR author or someone with write permission here needs to do it.

seanm · 2024-11-27T21:02:48Z

It's strange that HDF5, unlike say vtk and itk and other libraries, doesn't seem to have variables to just point to the zlib library file and headers folder. I'll take this up with them...

Created: HDFGroup/hdf5#5155

vfonov · 2024-11-27T22:19:07Z

Do you recall which?

strings.h is needed on linux.

seanm added 17 commits November 19, 2024 18:55

Fixed clang warnings about format string mismatches

835af26

Moved index check to before buffer access

791f2f8

Swapped the two halves of the && check. Safer to check the index first, before accessing the buffer. Out of range access was probably possible the old way.

Fixed clang -Wnewline-eof warnings

8f10c7f

Fixed clang -Warray-parameter warnings

e5cedaa

Several declarations and definitions didn't quite match.

Fixed clang -Wdeprecated-non-prototype warnings

eca7487

This was an antique K&R style function declaration.

Fixed most clang -Wmissing-prototypes warnings

4248861

Just marked private functions as static.

Fixed remaining -Wmissing-prototypes warnings by adding prototypes to…

2a20b21

… corresponding header

Fixed all clang -Wstrict-prototypes warnings

a063c0d

Unlike C++, the void is required here in C, strictly speaking.

Fixed some clang -Wunused-macros warnings

34d19eb

These were unused.

Removed checks for long-standard or long-obsolete header files

9849b33

Many of these header have been standardized since C89, others have been obsolete forever. This started as fixing some -Wundef warnings, but then I thought I'd clean up more.

Fixed one clang -Wfloat-conversion warning, variable type was just wrong

b40fb95

Fixed clang -Winfinite-recursion warning

6831c49

Indeed this was an infinite recursion. Looking at the functions above, and following that pattern, I think this meant to call the variant without the `2`.

Fixed clang -Wunused-variable warnings

1bc23d1

These variables were indeed unused, the sting not appearing elsewhere in the file.

Fixed only clang -Wmissing-noreturn warning

cc0fd7a

PrintVersion() called exit() meaning it never returned. There is only caller of PrintVersion(), so just moved the exit() call from inside to outside the function.

Fixed clang -Wuninitialized warnings

a0d30cc

Fixed clang -Wunused-function warning by wrapping function in #if 0

130629d

The prototype in the header was already in an #if 0.

Increased CMAKE_MINIMUM_REQUIRED from 3.6 to 3.10

6788ceb

Fixes warning "Compatibility with CMake < 3.10 will be removed from a future version of CMake." from CMake 3.31 Also moved it higher to fix "cmake_minimum_required() should be called prior to this top-level project() call" warning.

seanm changed the base branch from master to develop November 20, 2024 00:21

seanm mentioned this pull request Nov 26, 2024

Update CMake policies upto 3.10 #127

Closed

N-Dekker mentioned this pull request Nov 27, 2024

COMP: MINIC: Update CMake policies to 3.10 InsightSoftwareConsortium/ITK#4994

Closed

7 tasks

seanm closed this Nov 27, 2024

blowekamp mentioned this pull request Dec 6, 2024

COMP: libminc: address cmake version warning InsightSoftwareConsortium/ITK#5018

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes to various clang warnings, increased minimum CMake version #126

Fixes to various clang warnings, increased minimum CMake version #126

seanm commented Nov 20, 2024

seanm commented Nov 20, 2024

gdevenyi commented Nov 22, 2024

seanm commented Nov 22, 2024

seanm commented Nov 22, 2024

vfonov commented Nov 23, 2024

seanm commented Nov 24, 2024

vfonov commented Nov 25, 2024

seanm commented Nov 25, 2024

seanm commented Nov 25, 2024

gdevenyi commented Nov 26, 2024

vfonov commented Nov 26, 2024

dzenanz commented Nov 27, 2024

vfonov commented Nov 27, 2024

seanm commented Nov 27, 2024

dzenanz commented Nov 27, 2024

seanm commented Nov 27, 2024

vfonov commented Nov 27, 2024

Fixes to various clang warnings, increased minimum CMake version #126

Fixes to various clang warnings, increased minimum CMake version #126

Conversation

seanm commented Nov 20, 2024

seanm commented Nov 20, 2024

gdevenyi commented Nov 22, 2024

seanm commented Nov 22, 2024

seanm commented Nov 22, 2024

vfonov commented Nov 23, 2024

seanm commented Nov 24, 2024

vfonov commented Nov 25, 2024

seanm commented Nov 25, 2024

seanm commented Nov 25, 2024

gdevenyi commented Nov 26, 2024

vfonov commented Nov 26, 2024

dzenanz commented Nov 27, 2024

vfonov commented Nov 27, 2024

seanm commented Nov 27, 2024

dzenanz commented Nov 27, 2024

seanm commented Nov 27, 2024

vfonov commented Nov 27, 2024