Skip to content

[libc][fenv] Add compile time check #87826

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 1 commit into from
Apr 5, 2024

Conversation

robincaloudis
Copy link
Contributor

@robincaloudis robincaloudis commented Apr 5, 2024

Take care of a TODO. This
check makes sure that the fexcept_t
value fits in an int value.

TODO introduced in: 9550f8b

Take care of a TODO. This
check makes sure that the fexcept_t
value fits in an int value.
@llvmbot llvmbot added the libc label Apr 5, 2024
@llvmbot
Copy link
Member

llvmbot commented Apr 5, 2024

@llvm/pr-subscribers-libc

Author: Robin Caloudis (robincaloudis)

Changes

Take care of a TODO. This
check makes sure that the fexcept_t
value fits in an int value.


Full diff: https://github.com/llvm/llvm-project/pull/87826.diff

1 Files Affected:

  • (modified) libc/src/fenv/fegetexceptflag.cpp (+2-1)
diff --git a/libc/src/fenv/fegetexceptflag.cpp b/libc/src/fenv/fegetexceptflag.cpp
index 71b87ce7315d1a..c6160da7afbde2 100644
--- a/libc/src/fenv/fegetexceptflag.cpp
+++ b/libc/src/fenv/fegetexceptflag.cpp
@@ -15,7 +15,8 @@
 namespace LIBC_NAMESPACE {
 
 LLVM_LIBC_FUNCTION(int, fegetexceptflag, (fexcept_t * flagp, int excepts)) {
-  // TODO: Add a compile time check to see if the excepts actually fit in flagp.
+  static_assert(sizeof(int) >= sizeof(fexcept_t),
+                "fexcept_t value cannot fit in an int value.");
   *flagp = static_cast<fexcept_t>(fputil::test_except(FE_ALL_EXCEPT) & excepts);
   return 0;
 }

@michaelrj-google michaelrj-google requested a review from lntue April 5, 2024 20:47
@robincaloudis
Copy link
Contributor Author

@lntue, can you merge this PR? I lack write access to this repo. Thank you.

@lntue lntue merged commit 80deb82 into llvm:main Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants