This is the first step towards disentangling the debug mode and assertions
in libc++. This patch doesn't make any functional change: it simply moves
_LIBCPP_ASSERT-related stuff to its own file so as to make it clear that
libc++ assertions and the debug mode are different things. Future patches
will make it possible to enable assertions without enabling the debug
mode.
Details
- Reviewers
• Quuxplusone - Group Reviewers
Restricted Project - Commits
- rGf87aa19be644: [libc++] Move everything related solely to _LIBCPP_ASSERT to its own file
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
Seems reasonable to me.
libcxx/include/CMakeLists.txt | ||
---|---|---|
102 | Are we at all worried about name conflicts? (Or stylistically about adding new top-level detail headers?) Should this be something like __utility/libcpp_assert.h instead? | |
libcxx/include/__debug | ||
27 | This < allows people to set -D_LIBCPP_DEBUG_LEVEL=-1 without triggering the #error on line 32. | |
libcxx/src/assert.cpp | ||
19 | Consider std::to_string(__line_) for consistency with std::fprintf and std::abort on lines 27-28. |
libcxx/include/CMakeLists.txt | ||
---|---|---|
102 | I'm definitely in favor of moving that stuff into some directory. Maybe __libcpp/? That would most likely not clash with any new C++ headers and we could leave the libcpp_ prefix out. | |
libcxx/include/__debug | ||
27 | _LIBCPP_DEBUG_LEVEL should never be set directly. The public interface is _LIBCPP_DEBUG. |
libcxx/include/CMakeLists.txt | ||
---|---|---|
102 |
I don't want to get into a situation where <assert.h> means one thing and "./assert.h" means a different thing, so the prefix would need to stay even in that case. (Likewise, if we ever granularize <string>, I'll oppose the creation of __string/string.h. But luckily in that case it'll be __string/basic_string.h so the issue won't arise.) |
libcxx/include/CMakeLists.txt | ||
---|---|---|
102 | I agree that we should move those to a subdirectory, however I want to do that separately to give us ample time to bikeshed the name. We already have a "pattern" for adding these detail headers right now: it's to add them at the top-level. So I'm following the trend in this patch for the time being. | |
libcxx/include/__debug | ||
27 | I agree with Arthur, I'll fix it. This is an easy way to ensure that users don't define things they shouldn't define. |
Are we at all worried about name conflicts? (Or stylistically about adding new top-level detail headers?) Should this be something like __utility/libcpp_assert.h instead?
OTOH, maybe we keep it like this right now, and then at some point in the future we move __assert to __foo/libcpp_assert.h and __debug to __foo/debug_iterators.h in the same commit, for some value of __foo on which I'm sure we'll disagree. :)