LGTM.

In D35159#809724, @erik.pilkington wrote:

Rebase. I don't think the issue of purging underscores from this file/libcxx should block this, if we want to discuss that cfe-dev would probably be the place. I agree that it would be nice to clang-format this file, would anyone have any problems with me committing that after this? I found the git-blame of this file to be pretty useless, almost every line just points to r184097.
Are there any other thoughts on this change?

In D34331#803452, @EricWF wrote:

In D34331#803105, @dexonsmith wrote:

In D34331#802747, @EricWF wrote:

@dexonsmith Mind if I hijack this and check in your changes to <functional> with my tests?

Not at all.

In D34331#802821, @EricWF wrote:

@dexonsmith I'm not sure it's sane to allow reentrant behavior. Could you explain why you think it is?

It didn't seem sane to me at first either, despite this being supported by std::unique_ptr and std::shared_ptr. I found our user fairly convincing, though:

• They had an underlying reference counted object, so only the destruction of the last copy of A nulled out the pointer (mimicked that with the bool cancel).
• They had a callback function intended to be called once, and dropping that function on cancellation (mimicked that with a global variable). The cancel operation nulled out a std::function, but destroying the objects captured in the lambda in that std::function also separately decided to perform a cancel, which should have been OK. The cancel function just contained callback = nullptr.

According to [reentrancy] it is implementation defined what STL functions are allowed to be recursively reentered. I'm not opposed to providing reentrancy where it's useful, but we would have to provide it well.
This is where I was running into problems while I was writing tests. There are so many different ways you can reenter std::function's special members that it makes it impossible for an implementation
to truly support reentrancy as the standard would require.

If you consider that every possible copy construction or destruction of a user type is a potential reentrancy point, the complexity of having well-defined reentrant behavior starts to become clear.
Any time a copy constructor or destructor is invoked you have a potential reentrancy point which, in order to provide well defined behavior, must also act as a sort of _sequence point_ where the side effects of the current call are visible to the reentrant callers (For example your patches use of operator=(nullptr_t)).

The methods fixed in this patch are seemingly improvements; It's clear that operator=(nullptr) can safely be made reentrant. On the other hand consider operator=(function const& rhs), which is specified as equivalent to function(rhs).swap(*this).
I posit swap is not the kind of thing that can easily be made reentrant, but that making std::function assignment reentrant in general clearly requires it.

If fixing this bug is sufficiently important I'm willing to accept that, but I don't think it improves the status quo; We may have fixed a specific users bug, but we haven't made these functions safely reentrant (in fact most of the special members are likely still full of reentrancy bugs).

+mclow.lists

This LGTM. Mehdi, do you have any other concerns?

In D34331#802747, @EricWF wrote:

@dexonsmith Mind if I hijack this and check in your changes to <functional> with my tests?

Assuming nothing in arena<>...Db actually changed (just moved), this LGTM if you split the move into a separate prep commit.

In D34578#801519, @hfinkel wrote:

In D34578#801357, @dexonsmith wrote:

Ping! Hal, you committed r283051. Do you have a problem with this?

It looks like you're just renaming __libcpp_isnan and friends to __libcpp_isnan_or_builtin` and similar. LGTM

Ping! Hal, you committed r283051. Do you have a problem with this?

LGTM.

Ping!

In D34574#791158, @rsmith wrote:

Should we also remove the recently-added availability attributes in libc++ too? If I'm using a recent libc++ and providing my own aligned allocation functions, we shouldn't reject attempts to call those aligned allocation functions.

In D34574#791124, @ahatanak wrote:

Duncan and I had a discussion on this.

We are thinking about adding a warning that tells users that aligned allocation /deallocation operators are being called but they are not defined in the library.

LGTM.

I followed the other availability macros in using "strict", but I'm not sure this was the right decision. The documentation seems to recommend not using "strict" (it says that weakly-linking is almost always a better API choice).

libc++ is one of the exceptions. Use strict.

In D34446#787934, @arphaman wrote:

In D34446#787263, @dexonsmith wrote:

Right now, sys::getProcessTriple returns the LLVM_HOST_TRIPLE, whose system version might not be the actual version of the system on which the compiler running.

Are we sure no one depends on that behaviour?

The primary users of this function are the JIT libraries, but so far I haven't found any checks for the OS version in the JIT code. And it looks like most of the other users of this function don't check the OS version.

Right now, sys::getProcessTriple returns the LLVM_HOST_TRIPLE, whose system version might not be the actual version of the system on which the compiler running.

We use the -Wc++NN-compat- prefix on all the other subwarnings of -Wc++NN-compat warnings

I think -Wc++1z-mangling is best. IMO, with C++1z in the name, it's already clear that this is about compatibility. I also don't think we need to get as specific as -Wc++1z-compat-exception-spec.

Fixed in r305661.

Committed in r305647.

LGTM.

This is the right idea, although it only covers macOS.

You mention that the lock-free deque gives a speedup. I agree that should be left for a follow-up, but what kind of speedup did it give?

LGTM.

The current patch seems to be missing context on Phab (-U999999). Please remember to add it on the next iteration.

In D33513#765223, @dexonsmith wrote:

In D33513#765222, @mehdi_amini wrote:

If there is no other path to exercise LazyLoading of Metadata, let's just create one! (add an option to llvm-dis for example).

I agree.

In D33513#765222, @mehdi_amini wrote:

If there is no other path to exercise LazyLoading of Metadata, let's just create one! (add an option to llvm-dis for example).

I'm probably a good person to review this, but it's been a while since I've thought this so I might need help to refresh my context. This use-tracking code is unfortunately rather subtle.

Actually, looking through the comments, it appears that everyone (eventually) agreed with the approach in the patch. I agree too. LGTM.

Tests LGTM.

In D30633#756347, @chandlerc wrote:

In D30633#756320, @dexonsmith wrote:

As an aside, it seems the logic around unreachable (with and without the patch) might pessimize code paths that lead to noreturn functions, such as posix_spawn(). Is that really what we want? Why?

Heh, I thought about bringing this up, but it really seems orthogonal and something I wanted to look at.

Yeah, generally, the unreachable logic I wrote eons ago was dead wrong and this is still wrong in the case of nice "run forever" functions. I'll either send a patch or file a bug about this.

The testcase updates look good to me, and demonstrate the patch is doing what's intended. It might be worth adding a couple of tests for multiple branch targets (switch statements), since I'm not convinced everything adds up to 1 right now.

In D32724#750074, @vsk wrote:

In D32724#749868, @dexonsmith wrote:

In D32724#747728, @aprantl wrote:

Is it the right solution to use the module hash for correctness, or should the mismatch of the serialized langopts trigger a module rebuild and the module hash is only there to tune the performance/disk size tradeoff?

I'm not sure if there is (or should be) a hard-and-fast rule, but certainly in this case changing the hash SGTM. Otherwise, users toggling back and forth between build configurations would have to rebuild the modules each time.

Great, it looks like changing the module hash is acceptable. However, I'm not sure whether it's OK to make sanitizer feature mismatches errors for explicit modules. @aprantl suggests checking for language option mismatches early on instead of just relying on the module hash, but @rsmith mentioned:

I would expect this [sanitizer features] to be permitted to differ between an explicit module build and its use. (Ideally we would apply the sanitization settings from the module build to the code generated for its inline functions in that case, but that can wait.)

In D32724#747728, @aprantl wrote:

Is it the right solution to use the module hash for correctness, or should the mismatch of the serialized langopts trigger a module rebuild and the module hash is only there to tune the performance/disk size tradeoff?

Breaking the ABI of that configuration SGTM.

Thanks for being flexible; committed in r302095. Maybe longer term we could add a CMake configuration for whether to include the tests, but naming it might be hard ;).

LGTM.

(I must have misclicked somewhere.)

This LGTM too.

This LGTM, and it's liable to bitrot if it hangs out here any longer. We can always iterate in tree if we find a better way to organize the markup and/or tests.

Upgrade LGTM. (I'm not planning to review the rest, but let me know if you need me to take a closer look.)

rsmith: I just noticed you still have a red "X" here (since Phab won't let me "close"). I think I addressed your comments, but let me know if you want me to revert until you have time to look.

Thanks for the reviews! Committed in r301593.

Fixed in r301592.

(Both changes SGTM, but I doubt you're looking for my advice on the CMake logic.)

Oh, and it would be nice to split out Preprocessor::diagnoseMissingHeaderInUmbrellaDir in a separate NFC commit ahead of time.

LGTM after a couple of changes inline.

Thanks Eli and Richard.

Committed in r301060.

Since I haven't heard from Marshall and Hal's fine with the less-future-proof std::is_floating_point, I'll commit that and we can iterate in tree. Just running tests.

s/Eli/Eric/

Thanks for the review, Eli.

It looks like __INTPTR_TYPE__ was introduced in r64495 in order to help define intptr_t, and then they diverged in r89237.

Here's an updated patch that uses __SIZE_MAX__ and also handles the other pointer-like integers.

Hmm... presumably, this test should pass:

Marshall, are you fine with figuring this out in-tree? Or should I revert to using std::is_floating_point for now?

Thanks for the review. __SIZE_MAX__ makes sense to me; and better even if not for the #if requirement. I'll do that for both SIZE_MAX and PTRDIFF_MAX when I get a chance.

This was committed in some form a long time ago.

Eric: somehow I never got an email notification for your review :/. If Marshall is okay with the current state, I'll document that before commit.

Marshall, that's what I assumed originally, but I figured Hal had some non-standard-but-worth-supporting use case in mind.

Ping.