llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
26.78k stars 10.97k forks source link

Meaningless constraints (enable_if-s) on basic_string constructors #87368

Open AndreyG opened 3 months ago

AndreyG commented 3 months ago

Since the commit 76b26852b6be6e54c86741c7c80ca6b5d74eab2e some constructors of basic_string are "constrained" by enable_if which depends only on template parameters of the class basic_string itself. It's meaningless, and I'd say even harmful because it just slows down compilation but doesn't bring any value. For instance,

If _Allocator doesn't satisfy the requirement __is_allocator, then compilation fails during instantiation of basic_string (https://gcc.godbolt.org/z/7jbdrheov), it's not SFINAE error, because substitution of the type of NTTP parameter is not invoked lazily (https://gcc.godbolt.org/z/oaWbhbve9).

EricWF commented 3 months ago

I suspect that has to do with template deduction guides?

Those allocator constructors have given me problems elsewhere because they're too generic and soak up everything.

Can you articulate why string needs to support the case where it's instantiated with types that don't meet the requirements?

AndreyG commented 3 months ago

I suspect that has to do with template deduction guides?

Not in the first case, but maybe that's the reason for the second and third cases.

Can you articulate why string needs to support the case where it's instantiated with types that don't meet the requirements?

Of course, it doesn't need, it seems that I phrased my idea badly in the start message. I mean that these template headers

template <__enable_if_t<__is_allocator<_Allocator>::value, int> = 0>

don't work as constraints for the constructors (unlike, for example, template header of pair default constructor), because they are substituted not in SFINAE context. Instead of this it behaves like repeated multiple times validity check of the class template arguments, which if needed could be much better expressed as static_assert(__is_allocator<Allocator>::value) somewhere here.

frederick-vs-ja commented 3 months ago

These constraints are due to LWG3076, originally added in 76b26852b6be6e54c86741c7c80ca6b5d74eab2e (https://reviews.llvm.org/D48616).

The reason for the first case seems to be that libc++ splits the following constructor (constrained via LWG3076)

constexpr basic_string(const charT* s, const Allocator& a = Allocator());

into two overloads.

But the constraints seem redundant for the first split overload.

I mean that these template headers

template <__enable_if_t<__is_allocator<_Allocator>::value, int> = 0>

don't work as constraints for the constructors (unlike, for example, template header of pair default constructor), because they are substituted not in SFINAE context.

They (except for the first one) work in class template argument deduction.

AndreyG commented 3 months ago

WDYT about making the first constructor (with the single parameter of type const _CharT*) non-template?

  1. It doesn't break CTAD.
  2. It's the most often invoked constructor, because it takes part in implicit conversions where another constructors are filtered out by the number of parameters, and so for it optimization of compilation time is the most important.
EricWF commented 2 months ago

How much do having these slow down compilation?