Skip to content

Conversation

@ldionne
Copy link
Member

@ldionne ldionne commented Dec 1, 2025

Otherwise, they would use their own mechanism based on C assert. It's better to use the same assertion mechanism consistently everywhere since this code is considered an implementation detail of libc++.

…ion mechanism

Otherwise, they would use their own mechanism based on C assert. It's
better to use the same assertion mechanism consistently everywhere since
this code is considered an implementation detail of libc++.
@ldionne ldionne requested a review from a team as a code owner December 1, 2025 14:57
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Dec 1, 2025
@llvmbot
Copy link
Member

llvmbot commented Dec 1, 2025

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

Otherwise, they would use their own mechanism based on C assert. It's better to use the same assertion mechanism consistently everywhere since this code is considered an implementation detail of libc++.


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

1 Files Affected:

  • (modified) libcxx/src/include/from_chars_floating_point.h (+9-5)
diff --git a/libcxx/src/include/from_chars_floating_point.h b/libcxx/src/include/from_chars_floating_point.h
index 19eeeb28fb08d..20f23d2bc267d 100644
--- a/libcxx/src/include/from_chars_floating_point.h
+++ b/libcxx/src/include/from_chars_floating_point.h
@@ -9,11 +9,6 @@
 #ifndef _LIBCPP_SRC_INCLUDE_FROM_CHARS_FLOATING_POINT_H
 #define _LIBCPP_SRC_INCLUDE_FROM_CHARS_FLOATING_POINT_H
 
-// These headers are in the shared LLVM-libc header library.
-#include "shared/fp_bits.h"
-#include "shared/str_to_float.h"
-#include "shared/str_to_integer.h"
-
 #include <__assert>
 #include <__config>
 #include <cctype>
@@ -21,6 +16,15 @@
 #include <concepts>
 #include <limits>
 
+// Make sure we use libc++'s assertion machinery within the shared code we use
+// from LLVM libc.
+#define LIBC_ASSERT(cond) _LIBCPP_ASSERT((cond), _LIBCPP_TOSTRING(cond))
+
+// These headers are in the shared LLVM-libc header library.
+#include "shared/fp_bits.h"
+#include "shared/str_to_float.h"
+#include "shared/str_to_integer.h"
+
 // Included for the _Floating_type_traits class
 #include "to_chars_floating_point.h"
 

Copy link
Contributor

@philnik777 philnik777 left a comment

Choose a reason for hiding this comment

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

Is defining LIBC_ASSERT like this a supported feature?
It's probably important for us to have this be supported in some way, but let's make sure the libc folks are on board with this approach.

CC @michaelrj-google

@ldionne
Copy link
Member Author

ldionne commented Dec 1, 2025

Yes, this was the result of a feature request I made.

Copy link
Contributor

@michaelrj-google michaelrj-google left a comment

Choose a reason for hiding this comment

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

LGTM, this uses the mechanism established in #168087.

@ldionne ldionne merged commit 860146c into llvm:main Dec 1, 2025
82 checks passed
@ldionne ldionne deleted the review/use-libcxx-assert-from-libc branch December 1, 2025 20:17
kcloudy0717 pushed a commit to kcloudy0717/llvm-project that referenced this pull request Dec 4, 2025
…ion mechanism (llvm#170149)

Otherwise, they would use their own mechanism based on C assert. It's
better to use the same assertion mechanism consistently everywhere since
this code is considered an implementation detail of libc++.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants