Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unused variable warning in H5C.c #2844

Merged
merged 7 commits into from
May 9, 2023
Merged

Conversation

hyoklee
Copy link
Member

@hyoklee hyoklee commented Apr 28, 2023

Sunspot compiler (icx) throws a warning.

CMAKE_C_COMPILER:FILEPATH=/soft/restricted/CNDA/updates/2022.12.30.001/oneapi/\ cmpiler/trunk-20230201/compiler/linux/bin/icx

See https://my.cdash.org/viewBuildError.php?type=1&buildid=2327793 for details.

Sunspot compiler throws a warning.
@hyoklee
Copy link
Member Author

hyoklee commented Apr 28, 2023

This warning occurs with the following configuration:

cmake -DHDF5_BUILD_FORTRAN:BOOL=ON -DHDF5_ENABLE_MAP_API:BOOL=ON ..

src/H5C.c Outdated
@@ -6579,8 +6579,10 @@ H5C__make_space_in_cache(H5F_t *f, size_t space_needed, hbool_t write_permitted)
H5C_cache_entry_t *entry_ptr;
H5C_cache_entry_t *prev_ptr;
H5C_cache_entry_t *next_ptr;
uint32_t num_corked_entries = 0;
herr_t ret_value = SUCCEED; /* Return value */
#if !defined(SYCL_LANGUAGE_VERSION) || !defined(__INTEL_LLVM_COMPILER)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand how this can be undefined - it is incremented in line 6631. And the value is used in an assert at line 6758.

Copy link
Member Author

@hyoklee hyoklee Apr 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ask the developer who wrote H5C.c.
The responsible coder should understand this behavior.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is likely a false positive by the compiler based on the code below. And since it failed to compile on ubuntu when the variable was removed before #if !defined(SYCL_LANGUAGE_VERSION) || !defined(__INTEL_LLVM_COMPILER) was added, this will probably fail to compile for ICC in the absence of any compiler bugs. At a minimum, further uses of num_corked_entries below would need to also be surrounded by #if !defined(SYCL_LANGUAGE_VERSION) || !defined(__INTEL_LLVM_COMPILER), but I don't believe that's the correct fix.

@derobins derobins changed the title fix(lib): remove unused variable in H5C.c Remove unused variable in H5C.c Apr 28, 2023
@derobins derobins added Merge - To 1.12 Priority - 3. Low 🔽 Code cleanup, small feature change requests, etc. Component - C Library Core C library issues (usually in the src directory) Type - Improvement Improvements that don't add a new feature or functionality labels Apr 28, 2023
@hyoklee hyoklee changed the title Remove unused variable in H5C.c Remove unused variable warning in H5C.c Apr 28, 2023
@hyoklee
Copy link
Member Author

hyoklee commented Apr 28, 2023

The https://cs.illinois.edu/academics/courses/cs426 in 1996 reminded me how to fix it.

@jhendersonHDF
Copy link
Collaborator

jhendersonHDF commented Apr 28, 2023

The https://cs.illinois.edu/academics/courses/cs426 in 1996 reminded me how to fix it.

Hmm interesting, does ICX still give a warning if you use num_corked_entries++; rather than num_corked_entries = num_corked_entries + 1; or ++num_corked_entries? It's a little more compact and readable to use the post-increment and if ICC throws a warning for that then that seems like a fundamental compiler flaw.

@jhendersonHDF
Copy link
Collaborator

Regardless this is a compiler flaw so I don't see why we should be working around it.

@derobins derobins merged commit 80cf406 into HDFGroup:develop May 9, 2023
@qkoziol
Copy link
Contributor

qkoziol commented May 9, 2023

This was probably about the variable only being used for the later assert(). I have a fix for it in my current cache PR, so will resolve this issue there (and revert this particular change, probably).

qkoziol added a commit to qkoziol/hdf5 that referenced this pull request May 9, 2023
@hyoklee hyoklee deleted the patch-2 branch May 9, 2023 18:34
qkoziol added a commit that referenced this pull request May 9, 2023
* Add failure value where it's missing from 1+ macros.  Clean up
whitespace / continuation characters ('\').  Made hash-table macros generic
for use in both the package header and test header.  Remove duplicated
copy & pasted macros (by hoisting difference into #ifdef'd macro).
Updated and re-flowed comments to read better.

Also clean up a few compiler warnings in production builds.

Signed-off-by: Quincey Koziol <quincey@koziol.cc>

* Committing clang-format changes

* Remove unused variable warning in H5C.c (#2844)

* Remove trailing /* NDEBUG */ comment from #endif's

* Committing clang-format changes

---------

Signed-off-by: Quincey Koziol <quincey@koziol.cc>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
brtnfld pushed a commit to brtnfld/hdf5 that referenced this pull request May 17, 2023
brtnfld pushed a commit to brtnfld/hdf5 that referenced this pull request May 17, 2023
* Add failure value where it's missing from 1+ macros.  Clean up
whitespace / continuation characters ('\').  Made hash-table macros generic
for use in both the package header and test header.  Remove duplicated
copy & pasted macros (by hoisting difference into #ifdef'd macro).
Updated and re-flowed comments to read better.

Also clean up a few compiler warnings in production builds.

Signed-off-by: Quincey Koziol <quincey@koziol.cc>

* Committing clang-format changes

* Remove unused variable warning in H5C.c (HDFGroup#2844)

* Remove trailing /* NDEBUG */ comment from #endif's

* Committing clang-format changes

---------

Signed-off-by: Quincey Koziol <quincey@koziol.cc>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - C Library Core C library issues (usually in the src directory) Priority - 3. Low 🔽 Code cleanup, small feature change requests, etc. Type - Improvement Improvements that don't add a new feature or functionality
Projects
Status: Needs Merged
Development

Successfully merging this pull request may close these issues.

5 participants