I'm going to unblock this. Because I don't think it's harmful.

Great tests.

@philnik Please revert this change. It breaks a bunch of downstream users. We can discuss how or if to reapply it again later, but for the moment, please revert the change while we figure it out.
We appreciate your help unblocking downstream.

Did some digging, and it seems this should be fine. LGTM again.

Wait a second.

You need to describe the rational in the description.

Is the patch as uploaded correct? Because some bits appear to me as having disappeared.

LGTM.

In D150610#4343811, @ldionne wrote:

Pinging vendors for discussion.

In particular, @EricWF it would be nice to know how Google feels about this, given that it's a large code base built using -fno-exceptions (presumably the dylib too?).

If the bots are happy, then go ahead and revert+reland.
But if there are merge issues, then I think that forgetting the phab review number in the commit message is OK.

In D150284#4347476, @philnik wrote:

In D150284#4347301, @EricWF wrote:

The __threading header, which defines a stable API that libc++ uses to implement threading primitives. This has been used by a bunch of vendors to shim in the different implementations for their platforms.
I believe we'll want to do the same thing here.

Do we know what the full API will look like yet?

We don't know exactly which parts we will require, but I don't think that's relevant for this patch. We only implement a single (primitive) backend here, which will most likely not exist for a long time in this form.

I'm going to try rolling this change out at Google to see the scope of the breakage, how many bugs it finds, and what class of bugs those are.
I'll report back in a couple of days.

In D150300#4344047, @philnik wrote:

In D150300#4343616, @ldionne wrote:

In D150300#4343062, @Mordante wrote:

I also wonder whether we should look at removing the transitive includes in general (obviously in a separate patch). I expected some overhead from allowing them, but not these numbers. I still don't like it might break some users, but the benefits seem quite large.

Yeah, that's what this patch got me thinking about, too. This is kind of scary but it would likely provide worthwhile benefits to users. I do think landing a variant of this patch (aka where we change the default in the CI) is tied to removing the transitive includes by default for users, since we need to keep testing exactly what we ship, not something slightly different.

So if we were to remove the transitive includes for users, we'd want to give some grace period for users to update their code base, and give them a way to opt-in gradually into that change. So for example, we could:

1. Stop removing transitive includes under _LIBCPP_REMOVE_TRANSITIVE_INCLUDES, now when we remove transitive includes we start guarding it behind _LIBCPP_REMOVE_TRANSITIVE_INCLUDES_2.
2. Release-note that transitive includes will be removed by default in LLVM 18 (for example), and they can define _LIBCPP_REMOVE_TRANSITIVE_INCLUDES in LLVM 17 to start preparing their code base.
3. In LLVM 18, we flip the default to remove transitive includes and optionally we provide a way to opt into the old behavior with a macro like _LIBCPP_PROVIDE_TRANSITIVE_INCLUDES. This is the grace period.

IMO it would make more sense to add a new macro, since the users who already define _LIBCPP_REMOVE_TRANSITIVE_INCLUDES are fine with the transitive includes not being stable.

4. In LLVM 19, we remove the escape hatch and unconditionally remove all transitive includes marked under _LIBCPP_REMOVE_TRANSITIVE_INCLUDES.
5. Now we begin the same cycle again but with _LIBCPP_REMOVE_TRANSITIVE_INCLUDES_2.
6. Optionally, we add a #warning if _LIBCPP_REMOVE_TRANSITIVE_INCLUDES is defined just so that people who used that macro to opt-in their code bases earlier can clean up their build scripts.

In all cases, we can never reuse the _LIBCPP_REMOVE_TRANSITIVE_INCLUDES name unless we know nobody's defining the macro anymore because otherwise we could break them.

Given that we already granularized most of the relevant stuff I wonder whether it's even worth it to start a new cycle instead of just removing includes unconditionally after we've done it once. After that way fewer people will be broken by removing transitive includes. I also can't find many more headers which we would want to granulize. My remaining candidates are <math.h>, <mutex>, <locale> and possibly <future>, <ios>, <istream> and <regex>. AFAIK everything else is already granularized, a single-class header or quite small.

I'm with ldionne that we should keep using optional here. The correctness seems like a very important quality that we'll be loosing some certainty in.

Do we have -ftime-trace files for this change?

What semantic requirements do these violate. Please add that information to the description.

The __threading header, which defines a stable API that libc++ uses to implement threading primitives. This has been used by a bunch of vendors to shim in the different implementations for their platforms.
I believe we'll want to do the same thing here.

Committed in db381405ced7b65ad1cf8fc60bbdfb505e1ed3de

If we think there's no need for the friend declaration, then lets explain why we think it's unneeded in the commit message for archeology.

Sorry, I meant to comment that this is likely a misspelling of __slice_expr. Though I don't know if it's needed, and the test coverage here is really poor. But I would rather change it to what the author likely intended it to be.

LGTM, but I would wait for another LGTM.

OK, lets try this again. Hopefully with cooler heads.

Can you please update the patch so that it merges?

Have we done any benchmarking on this? I know the optimizer can be quite sensitive to the exact formulation of the loop.

Oh, and of course, after the CI is passing. Please re-upload a new patch so that it merges.

LGTM after addressing @phosek comment about the other delete functions.

@philnik I believe your actions here are unbecoming. I would ask you to reconsider the way you interact in the community.

In D146190#4256345, @philnik wrote:

In D146190#4256333, @EricWF wrote:

I think this is ready. It LGTM. Lets just get the CI passing.

Please wait for other people who have already commented. It's really not nice to overrule people like that, and this isn't exactly time sensitive.

I think this is ready. It LGTM. Lets just get the CI passing.

OK, so I've come around on this change. But please give me 2 weeks to cleanup the breakages it's going to cause inside Google.
I'll come back and let you know how many breakages were bugs vs how many were just weird meta-programming.

I think this is a bandaid over a bigger clang bug.

Thanks for updating the description. With rational, this makes a ton of sense.

Rereading the bug on github. I think this might deserve a standard defect report.

I think this is a really bad idea.
Splitting the specialization out so that only some files can depend on it, while others cannot, allowing some bits of the code to only see a partial implementation is going to be bug prone.

Has this patch been tested against Chromium? Has it been tested elsewhere?
The more testing it's undergone, the more confident I can be.

I agree with @Mordante

Is this code ODR used? I suspect it is, and so it's not valid, but I'm not sure and I ran into a bunch of different instances of this pattern while testing this change.

Nice tests. Big fan of the work done in this patch.

Why? If we've done everything correctly, there should be no difference between the two type? What's the motivation for this?

Disregard my suggestion about unavailable. It seems to always fire, including in unevaluated contexts. I feel silly.

Just use __attribute__((unavailable("declval cannot be called")) or some such.

Also, I'm not sure a static assert pointing to the stable name in the
standard is helpful to our users. Will they even know what it is?

We went out of our way to make declval as low cost as we possibly could. Adding a body for the compiler to see is just work.

Cool deal. Thanks for working on this.

@philnik Every single change needs a description. The description must include the motivation for making the change. What does in improve? What code does it break/fix? etc.

This increases code duplication substantially.

How is libc++ not working with it yet a blocker for them? We disable warnings in our headers except outside of our build?

Is there a design document describing these sets of changes?
If this affects the library *build* primarily, then it should be possible to make this continue to work.

This change is not correct and cannot proceed.

This cleanup seems incomplete. Why keep __less as a template at all given it's never actually used as one?

I'm not convinced this is something that we want to add the complexity for.

Changes need descriptions.

In D135248#3919379, @phyBrackets wrote:

Hey @EricWF , could you please take a look at this patch, i guess there are some tests which got affected by this . I'm not sure if i need to update them for passing the CI?

Why is this a cleanup?

This changes behavior.

Do you know why the hint wasn't initially used? Was there a point where compilers didn't actually provide the hints?

Sorry, I'm going to have to let @ldionne weigh in on this too. I'm just not sure introducing this whole new build mode with a separate exception library is something we want to do -- so I would like input from the other maintainers.

I don't really like the state this leaves variant, any and friends in when exception vtables aren't compiled into the dylib.
There's no way to know only from the headers that using any of these types will result in linker errors.

In D124516#3903536, @huixie90 wrote:

In D124516#3900680, @EricWF wrote:

Another improvement that can be made is reducing the number of instantiations we perform in a lot of the metaprogramming. _And, _Not, and _Or should only be used when short circuiting is needed. In most cases here it isn't. Every single instantiation performed in expected is carried around by the compiler and in the AST for the entire translation unit. That's costly, lets avoid that cost.

I've removed short circuiting for the static_assert cases. but most meta programming in this patch can benefit from short-circuiting as they are used in the constructors' constraints. (the most complicated one in this patch is the __can_convert @ldionne had some comments explaining why they are needed. (in the early revisions of this patch, there was no short-circuiting)

That being said, I wrote sine simple tests that benchmark the compilation times.

The first one is the case where short circuiting happens

// The 3rd requirement of `__can_convert`
// _Not<is_constructible<_Tp, expected<_Up, _Gp>&>>
// fails and short circuits
template <std::size_t N>
struct Foo {
  Foo() {}

  template <std::size_t M>
  Foo(Foo<M>) {}

  template <std::size_t M, class E>
  Foo(std::expected<Foo<M>, E>) {}
};

template <std::size_t N>
void callCtor() {
  std::expected<Foo<N>, int> e1;
  [[maybe_unused]] std::expected<Foo<N + 1>, int> e2(e1);
}

template <std::size_t... Ns>
void generateTest(std::index_sequence<Ns...>) {
  (callCtor<Ns>(), ...);
}

int main() { generateTest(std::make_index_sequence<1000>{}); }

• With the _And/_Or version, it took *8.9s* to compile.
• With the _BoolConstant<is_xxx_v && ...> version, it took *10.0s* to compile

So we see some benefits of short circuiting (out weighs the instantiation cost)

Then I have another test case where all boolean requirements pass so no short circuiting happening

// All requirements of `__can_convert` pass
// short circuiting has no benefit
template <std::size_t N>
struct Foo {
  Foo() {}

  template <std::size_t M>
  Foo(Foo<M>) {}
};

template <std::size_t N>
void callCtor() {
  std::expected<Foo<N>, int> e1;
  [[maybe_unused]] std::expected<Foo<N + 1>, int> e2(e1);
}

template <std::size_t... Ns>
void generateTest(std::index_sequence<Ns...>) {
  (callCtor<Ns>(), ...);
}

int main() { generateTest(std::make_index_sequence<1000>{}); }

• With the _And/_Or version, it took *8.5s* to compile.
• With the _BoolConstant<is_xxx_v && ...> version, it took *7.3s* to compile

So we know that short circuiting doesn't help because all booleans are true. And the instantiation costs make it slower.

The first case we gain 1.1s and the second case we lost 1.2s. So I am on the boarder line of whether or not to do short circuiting on these constructor constraints. I think in the real case, it is likely that those booleans are all true because if the user calls these converting constructors, it is likely to succeed instead of falling back to the constructor that takes U&&. That means in real user code, the short circuiting might never happen.

Stop being clever in tests. These changes add a substantial cognitive load to any person who's trying to read them for the first time and understand what they do.

This is from clang-format? LGTM then.

I don't think the test suite is the place to get clever with code deduplication and template magic.

Disregard me. They were all marked inline already.

This also affects external instantiations. So I'm not sure this is as simple as just "removing boilerplate".

Sorry for the repeated comments. But if I understand the description correctly, the diagnostic below is no longer emitted?

I would like other people to weigh in on this patch. I don't think these changes pull their weight. I'm not convinced they add clarity. And I'm certain they add complexity and cost -- also if we get the assert wrong (and I don't think we will), but we risk breaking totally valid code that would otherwise compile.

I'm not convinced your average user knows what copy insertable means, and I think the static assert is more opaque than the templated messages, which while noiser, contain more information.

This change doesn't improve conformance, right? It just attempts to make the diagnostics nicer in a handful of cases where the error occurs in the immediate function call to the allocator?

In D137127#3900823, @rprichard wrote:

In D137127#3900589, @EricWF wrote:

I don't think this helps, but I don't think it hurts. The correctness of the tests requires std::nullptr_t to be decltype(nullptr), so /shrug

The question is whether the <cstdlib> header is allowed to define a ::nullptr_t. If it does define it (it does with the Android NDK), then the test doesn't compile because -Wshadow complains about the local nullptr_t. (I suspect the issue is that bionic's stdlib.h includes malloc.h, which includes stddef.h without first declaring any __need_* macros. When libc++'s stddef.h is included, it defines ::nullptr_t.) It could be an issue with the bionic headers, but that's also probably hard to clean up without breaking stuff?