This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Move everything related solely to _LIBCPP_ASSERT to its own file
ClosedPublic

Authored by ldionne on Feb 14 2022, 1:00 PM.

Details

Summary

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.

Diff Detail

Event Timeline

ldionne created this revision.Feb 14 2022, 1:00 PM
ldionne requested review of this revision.Feb 14 2022, 1:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 14 2022, 1:00 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone added a subscriber: Quuxplusone.

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?
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. :)

libcxx/include/__debug
28

This < allows people to set -D_LIBCPP_DEBUG_LEVEL=-1 without triggering the #error on line 32.

libcxx/src/assert.cpp
20

Consider std::to_string(__line_) for consistency with std::fprintf and std::abort on lines 27-28.

philnik added inline comments.
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
28

_LIBCPP_DEBUG_LEVEL should never be set directly. The public interface is _LIBCPP_DEBUG.

libcxx/include/CMakeLists.txt
102

and we could leave the libcpp_ prefix out.

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.)

ldionne marked 6 inline comments as done.Feb 15 2022, 9:11 AM
ldionne added inline comments.
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
28

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.

ldionne updated this revision to Diff 409048.Feb 15 2022, 1:55 PM
ldionne marked 2 inline comments as done.

Rebase and fix a couple of issues

ldionne updated this revision to Diff 409232.Feb 16 2022, 6:42 AM

Rebase again and hopefully fix all CI issues. I'm going to land this if CI is green.

ldionne accepted this revision as: Restricted Project.Feb 16 2022, 6:42 AM
This revision is now accepted and ready to land.Feb 16 2022, 6:42 AM