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

toolchain: Replace GCC_VERSION and BUILD_ASSERT macros in gcc.h #51911

Merged

Conversation

keith-packard
Copy link
Collaborator

GCC_VERSION is defined in a few modules, and those headers are often included first, so we need to replace that definition with the Zephyr-specific one.

BUILD_ASSERT is also defined in include/toolchain/common.h, which might get included before gcc.h. We want to use the gcc-specific one instead of the general one.

Signed-off-by: Keith Packard keithp@keithp.com

Copy link
Member

@stephanosio stephanosio left a comment

Choose a reason for hiding this comment

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

As per #51963, the modules should either conditionally define (if the semantics are equivalent to that of Zephyr) or rename; not the other way around.

@keith-packard
Copy link
Collaborator Author

As per #51963, the modules should either conditionally define (if the semantics are equivalent to that of Zephyr) or rename; not the other way around.

#51963 seems to suggest that we use Z_GCC_VERSION in this file as it is purely internal in nature and not part of the Zephyr API. I'll run tests with BUILD_ASSERT left in place, but with Z_GCC_VERSION and see what happens. This patch is getting old enough that "things have changed".

@stephanosio
Copy link
Member

#51963 seems to suggest that we use Z_GCC_VERSION in this file as it is purely internal in nature and not part of the Zephyr API. I'll run tests with BUILD_ASSERT left in place, but with Z_GCC_VERSION and see what happens. This patch is getting old enough that "things have changed".

Sure, GCC_VERSION is not part of any public API and is more or less only intended to be used by the "toolchain abstraction layer," so Z_GCC_VERSION would not be a bad choice; though, the Z_ prefix is often more associated with the kernel and arch-layer stuff.

Since this is a toolchain abstraction layer-specific macro, TOOLCHAIN_GCC_VERSION might not be a bad choice either -- prefixing subsystem-specific identifiers with a subsystem-specific keyword is always a good practice.

GCC_VERSION is defined in a few modules, and those headers are often
included first, so replace the one used in zephyr with
TOOLCHAIN_GCC_VERSION. Do the same with CLANG_VERSION, replacing it with
TOOLCHAIN_CLANG_VERSION.

BUILD_ASSERT is also defined in include/toolchain/common.h, which might
get included before gcc.h. We want to use the gcc-specific one instead
of the general one.

Signed-off-by: Keith Packard <keithp@keithp.com>
@keith-packard
Copy link
Collaborator Author

Since this is a toolchain abstraction layer-specific macro, TOOLCHAIN_GCC_VERSION might not be a bad choice either -- prefixing subsystem-specific identifiers with a subsystem-specific keyword is always a good practice.

Sounds good. I've rebased and pushed this version.

@@ -64,6 +64,7 @@
#endif


#undef BUILD_ASSERT /* clear out common version */
Copy link
Member

Choose a reason for hiding this comment

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

BUILD_ASSERT is also defined in include/toolchain/common.h, which might
get included before gcc.h. We want to use the gcc-specific one instead
of the general one.

toolchain/common.h is never supposed to be included before the toolchain-specific headers such as toolchain/gcc.h; in fact, it should not be included by any files other than the toolchain-specific headers inside toolchain/.

Properly documenting and enforcing that seems to be beyond the scope of this PR, however.

@carlescufi carlescufi merged commit c58c76e into zephyrproject-rtos:main Dec 5, 2022
@keith-packard keith-packard deleted the override-gcc-macros branch December 25, 2022 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants