Skip to content

ompi: rename assert variables to mpi_assert #8591

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

Conversation

hjelmn
Copy link
Member

@hjelmn hjelmn commented Mar 11, 2021

There already exists an identifier with the name assert. It is not
appropriate to use this name for variables in C. This commit changes
these variable to mpi_assert. This should address an issue where
clang-format gives the wrong result when doing bit-wise operations on
these variables.

Signed-off-by: Nathan Hjelm [email protected]

There already exists an identifier with the name assert. It is not
appropriate to use this name for variables in C. This commit changes
these variable names to mpi_assert. This should address an issue where
clang-format gives the wrong result when doing bit-wise-and operations
on these variables.

Signed-off-by: Nathan Hjelm <[email protected]>
@hjelmn hjelmn force-pushed the for_the_love_of_all_things_DO_NOT_NAME_VARIABLES_ASSERT branch from 9387dab to aaabe3d Compare March 11, 2021 19:53
Copy link
Contributor

@rhc54 rhc54 left a comment

Choose a reason for hiding this comment

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

I'm amazed the compiler would let people get away with it!

@awlauria
Copy link
Contributor

bot:aws:retest

one more time...

Copy link
Contributor

@devreal devreal left a comment

Choose a reason for hiding this comment

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

Fine with me since it works around an issue with clang-format.

For the record: using assert as a variable name is perfectly. The C assert is a function-like macro that is only expanded when called. Otherwise the pre-compiler will not touch it. See https://gcc.gnu.org/onlinedocs/cpp/Function-like-Macros.html

@hjelmn
Copy link
Member Author

hjelmn commented Mar 11, 2021

@devreal That makes sense. Wasn't sure how it is defined but the C11 standard clearly says assert() is a macro not a function. Despite this it is generally discouraged to shadow an identifier. Would be nice if we could make gcc error on this so we don't run into any other issues.

@hjelmn
Copy link
Member Author

hjelmn commented Mar 11, 2021

Not sure what is going on with the build checker. Kicking it again

bot:aws:retest

@awlauria
Copy link
Contributor

@hjelmn There is a -Wshadow option for gcc that will generate a warning for shadowing (not sure about other compilers). I can add it to the CFLAGS, but I have a feeling it will create a dump of warnings. I have some cycles to clean it up though.

@hjelmn
Copy link
Member Author

hjelmn commented Mar 11, 2021

@awlauria That sounds like a good idea. Generally, shadowing leads to inadvertent errors. If gcc is not already warning we should turn that on.

@hjelmn
Copy link
Member Author

hjelmn commented Mar 11, 2021

Though thinking about it -Wshadow may not help here since assert is a macro.

@awlauria
Copy link
Contributor

Yeah unfortunately it won't pick up the assert() case, but still a good thing to scrub.

@hjelmn
Copy link
Member Author

hjelmn commented Mar 11, 2021

Ugh. The build checker is busted.

@awlauria awlauria mentioned this pull request Mar 11, 2021
@hjelmn
Copy link
Member Author

hjelmn commented Mar 11, 2021

Looked the build checker and everything worked fine except for a random failure in the clang38 workflow. Not related to this change.

@hjelmn hjelmn merged commit 037b40b into open-mpi:master Mar 11, 2021
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.

4 participants