Page MenuHomePhabricator

[libcxx] Simplify conditional definition of _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
AbandonedPublic

Authored by ldionne on Dec 17 2018, 2:34 PM.

Details

Reviewers
EricWF
Summary

It turns out that _LIBCPP_HAS_NO_ALIGNED_ALLOCATION can never be defined
before this point (because that is the only place that defines it), so
the conditional definition of _LIBCPP_HAS_NO_ALIGNED_ALLOCATION can be
simplified.

Event Timeline

ldionne created this revision.Dec 17 2018, 2:34 PM

I'm putting up a review for this simple change in case the intent was to allow users to define _LIBCPP_HAS_NO_ALIGNED_ALLOCATION before including a libc++ header. I don't expect that was the intent, but just in case.

If that happens to be true, I think that intent should be documented and a CMake #define should be added. I'd also like to understand why that use case is something we need to support.

In general, we *do* protect against user-defined defines.
This, and defining stuff as negative (_LIBCPP_HAS_NO_XXX) lets users disable parts of the libc++ functionality by defining the macros.

So I think we should leave this "as is"

In general, we *do* protect against user-defined defines.
This, and defining stuff as negative (_LIBCPP_HAS_NO_XXX) lets users disable parts of the libc++ functionality by defining the macros.

So I think we should leave this "as is"

The problem I have with this is that we're hence implicitly supporting this use case, which means we should make sure we don't break it, we should add a tester with this configuration, and we should document that this macro can be used to alter the behaviour of the library. Either we should do this if we have a reason to, or we should remove this accidental API by not checking whether the user defines the macro. Does this make sense?

In general, we *do* protect against user-defined defines.
This, and defining stuff as negative (_LIBCPP_HAS_NO_XXX) lets users disable parts of the libc++ functionality by defining the macros.

So I think we should leave this "as is"

The problem I have with this is that we're hence implicitly supporting this use case, which means we should make sure we don't break it, we should add a tester with this configuration, and we should document that this macro can be used to alter the behaviour of the library. Either we should do this if we have a reason to, or we should remove this accidental API by not checking whether the user defines the macro. Does this make sense?

90 seconds of grepping found the following extant macros in <__config>: _LIBCPP_HAS_NO_ALIGNED_ALLOCATION, _LIBCPP_HAS_NO_ASAN, _LIBCPP_HAS_NO_ATOMIC_HEADER, _LIBCPP_HAS_NO_AUTO_TYPE, _LIBCPP_HAS_NO_BUILTIN_ADDRESSOF, _LIBCPP_HAS_NO_BUILTIN_OPERATOR_NEW_DELETE, _LIBCPP_HAS_NO_CONSTEXPR, _LIBCPP_HAS_NO_COROUTINES, _LIBCPP_HAS_NO_CXX14_CONSTEXPR, _LIBCPP_HAS_NO_CXX20_CHRONO_LITERALS, _LIBCPP_HAS_NO_DECLTYPE, _LIBCPP_HAS_NO_DEDUCTION_GUIDES, _LIBCPP_HAS_NO_GENERALIZED_INITIALIZERS, _LIBCPP_HAS_NO_GLOBAL_FILESYSTEM_NAMESPACE, _LIBCPP_HAS_NO_INT128, _LIBCPP_HAS_NO_IS_AGGREGATE, _LIBCPP_HAS_NO_LAMBDAS, _LIBCPP_HAS_NO_LIBRARY_ALIGNED_ALLOCATION, _LIBCPP_HAS_NO_LONG_LONG, _LIBCPP_HAS_NO_MONOTONIC_CLOCK, _LIBCPP_HAS_NO_NOEXCEPT, _LIBCPP_HAS_NO_NULLPTR, _LIBCPP_HAS_NO_OFF_T_FUNCTIONS, _LIBCPP_HAS_NO_PRAGMA_PUSH_POP_MACRO, _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER, _LIBCPP_HAS_NO_RVALUE_REFERENCES, _LIBCPP_HAS_NO_SPACESHIP_OPERATOR, _LIBCPP_HAS_NO_STDIN, _LIBCPP_HAS_NO_STDOUT, _LIBCPP_HAS_NO_STRONG_ENUMS, _LIBCPP_HAS_NO_THREAD_UNSAFE_C_FUNCTIONS, _LIBCPP_HAS_NO_THREADS, _LIBCPP_HAS_NO_UNICODE_CHARS, _LIBCPP_HAS_NO_VARIABLE_TEMPLATES, _LIBCPP_HAS_NO_VARIADICS, _LIBCPP_HAS_NO_VECTOR_EXTENSION.

As far as I know, we don't have testers for all of these.

What I'm saying is that this is a pattern we use throughout libc++, and I'd rather follow that pattern than explain why _LIBCPP_HAS_NO_ALIGNED_ALLOCATION is different from all these others.

Ok, some of those we define ourselves w/o checking.

ldionne abandoned this revision.Jan 7 2019, 10:52 AM

What I'm saying is that this is a pattern we use throughout libc++, and I'd rather follow that pattern than explain why _LIBCPP_HAS_NO_ALIGNED_ALLOCATION is different from all these others.

Fair enough.