Skip to content

Fix some compiler warnings #12685

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

Merged
merged 4 commits into from
Aug 10, 2024
Merged

Conversation

devreal
Copy link
Contributor

@devreal devreal commented Jul 18, 2024

Random selection of compiler warnings found with Clang 17.

The warnings addressed here:

pialltoallw_f.c: In function ‘ompi_ialltoallw_f’:
pialltoallw_f.c:110:12: warning: ‘c_sdispls’ may be used uninitialized in this function [-Wmaybe-uninitialized]
     c_ierr = PMPI_Ialltoallw(sendbuf,
     ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~
                              OMPI_ARRAY_NAME_CONVERT(sendcounts),
                              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                              OMPI_ARRAY_NAME_CONVERT(sdispls),
                              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                              c_sendtypes,
                              ~~~~~~~~~~~~
                              recvbuf,
                              ~~~~~~~~
                              OMPI_ARRAY_NAME_CONVERT(recvcounts),
                              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                              OMPI_ARRAY_NAME_CONVERT(rdispls),
                              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                              c_recvtypes, c_comm, &c_request);
                              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
pialltoallw_f.c:110:12: warning: ‘c_sendcounts’ may be used uninitialized in this function [-Wmaybe-uninitialized]
../../../../ompi/mca/coll/base/coll_base_allreduce.c: In function ‘ompi_coll_base_allreduce_intra_allgather_reduce’:
../../../../ompi/mca/coll/base/coll_base_allreduce.c:1284:9: warning: unused variable ‘rank’ [-Wunused-variable]
     int rank = ompi_comm_rank(comm);
         ^~~~
../../../../../opal/mca/smsc/xpmem/smsc_xpmem_component.c: In function ‘mca_smsc_xpmem_component_query’:
../../../../../opal/mca/smsc/xpmem/smsc_xpmem_component.c:119:19: warning: variable ‘low’ set but not used [-Wunused-but-set-variable]
         uintptr_t low, high;
                   ^~~

@devreal devreal requested a review from bosilca July 18, 2024 12:39
Copy link
Member

@bosilca bosilca left a comment

Choose a reason for hiding this comment

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

The commits are not signed. Don't merge before fixing this.

Copy link
Member

@bosilca bosilca left a comment

Choose a reason for hiding this comment

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

sign the commits

@devreal
Copy link
Contributor Author

devreal commented Jul 18, 2024

They are signed with a Signed-off-by line but no cryptographically. I can try to get me the verified tag but I don't think we have required that in the past

@bosilca
Copy link
Member

bosilca commented Jul 18, 2024

I thought we did, but after checking the last set of commits it appears we haven't. Most of the regular contributors are either "Verified" or nothing. But somehow you are specifically marked as "Unverified".

devreal added 4 commits July 18, 2024 11:32
Signed-off-by: Joseph Schuchart <joseph.schuchart@stonybrook.edu>
Signed-off-by: Joseph Schuchart <joseph.schuchart@stonybrook.edu>
Signed-off-by: Joseph Schuchart <joseph.schuchart@stonybrook.edu>
The compiler will otherwise complain about potentially uninitialized variables.

Signed-off-by: Joseph Schuchart <joseph.schuchart@stonybrook.edu>
@devreal devreal force-pushed the fix-compiler-warnings branch from 3270274 to 41a1a2f Compare July 18, 2024 15:33
@devreal
Copy link
Contributor Author

devreal commented Jul 18, 2024

I think I have enabled that warning for my commits. Just added my new email address to my GPG key and now it shows as Verified (applies to my prior commits as well)

@jsquyres
Copy link
Member

@devreal is right: we've just required the signoff line in the commit message -- we haven't required cryptographically-verified commits.

Copy link
Member

@jsquyres jsquyres left a comment

Choose a reason for hiding this comment

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

@bosilca You have a negative review based on asking @devreal to sign his commits. Can you change that to a positive review?

Thanks!

@bosilca bosilca merged commit c3bebd8 into open-mpi:main Aug 10, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants