LGTM as well. Thanks for working on this!

IDK if I know what's causing the issue, but this change LGTM.

In D80376#2101677, @ldionne wrote:

In D80376#2101587, @EricWF wrote:

This is not the ABI we want.

Are you thinking of a single pointer to information stored in the data segment by the compiler?

This is not the ABI we want.

LGTM

LGTM.

How do we implement the "Mandates" requirement specified here?

std::any, like other type erased types, cannot follow the allocator model and therefore does not use allocators.

All of that being said, this code is correct and LGTM**.

Why is this code an improvement?

@mvels Do we have macro-benchmark results for this change?

I'm a bit confused by the build size numbers you gave compared to the patch.

We could lift the two macros that depend on _NEWLIB_VERSION into the headers that use them.
Then we'll avoid the include in config.

LGTM.
I can't find any usages of this internally.

In D78200#2023637, @broadwaylamb wrote:

In D78200#2023624, @smeenai wrote:

Sorry for jumping in without context, but wanted to chime in on one thing.

Windows doesn't really have a concept of symlinks. So, when the monorepo is cloned, those symlinks turn to ordinary text files. Previously, if we cross-compiled libc++ for some symlink-friendly system (e. g. Linux) and ran tests on the target system, some tests would fail.

This isn't right. Windows has supported symbolic links since Vista. The big limitation was that you needed to be in an elevated process to be able to create symbolic links. Windows 10 lifted that limitation in Developer Mode starting with the April 2017 release (https://blogs.windows.com/windowsdeveloper/2016/12/02/symlinks-windows-10/). If git for Windows isn't creating symbolic links properly, that's a git issue and not a Windows issue (although I recognize it doesn't make much difference for the end user). Some searching suggests that you should be able to configure Git on Windows to handle symlinks correctly (https://github.com/libgit2/libgit2/pull/4713, and https://github.community/t5/How-to-use-Git-and-GitHub/git-bash-symbolic-links-on-windows/m-p/11488#M3732 shows an option to enable symbolic links when running the setup).

I am aware of this, and I even had some success turning on support for symlinks on Windows. Which gives me the right to say that this process requires a little more involvement than it should :-)

However, things are even more complicated on the buildbot (which is the main purpose of this effort). Namely, there are some limitations of the buildbot platform that won't allow passing custom flags to Git when cloning the repo.

LGTM.

If we don't want the static env, we should remove it entirely.
Meaning, we should convert each of these tests to use dynamic_test_env and setup the required files like the remainder of the tests do.

In D78200#2022470, @broadwaylamb wrote:

I'm sorry, I've messed up. I first committed this as rG645ad5badbabdeca31de5c98ea8135c5a6e7d710, but then realized that I've committed the previous version of this patch. So I reverted it and recommitted as 52cc8bac7780dbfb90dcc8cfe24696618eeaa06e.

In D78200#2021841, @EricWF wrote:

That said, I don't understand how this patch makes these tests pass on Windows.
If Windows doesn't support symlinks, how does dynamically creating them make a difference?

This patch is not about making them pass on Windows, but rather about making them pass on Linux in the case when we cross-compile them on Windows.

While I support this change, it should really be accompanied by a library working group issue or paper.

Unless I'm mistaken, this change is racy. Tests in different files are run in parallel.
One test could be tearing down the static environment while another in trying to construct it.

When I initially wrote the filesystem tests, I had divided them between "static" and "dynamic"; that is, tests which modified the target filesystem and those which did not.

There are some simplifications suggested in the inline comments.
Once those are addressed, this LGTM.

af2968e37f4c95846ffe287b64a4fcd72c765bee

In D78323#1987596, @sdesmalen wrote:

LGTM, did this comparison introduce any non-determinism?

The entire point of the static test environment is that it's /static/.
Meaning it's checked in and not created on the fly.

In D78245#1985201, @thakis wrote:

Are you aware of https://llvm.org/docs/CommandGuide/lit.html#substitutions ("%T: parent directory of %t (not unique, deprecated, do not use)")? We've talked about this a bunch in the past, and the decision back then was to do mkdir %t in the tests that need a dir, and to remove %T over time.

This needs a bunch of additional tests. Specifically of the passing kind :-)

I assume this is a behavioral change? Why are there no new tests?

Committed as 601f7631827ae6ac08117a282c83a62b67dedf48

I have some concerns with how we harden our concepts,
but these can be addressed in follow up commits.

I like tests.
Assuming all of them pass;: LGTM.

I don't necessarily agree.

Lets not start writing tests like this.

We don't implement non-standard extensions in libc++.
Only when this trait is proposed for standardization can we consider adding it to the library

I would like us to be even more aggressive bumping our compiler support,
but that's a conversation for another time.

In D76256#1930417, @ldionne wrote:

In D76256#1930359, @EricWF wrote:

These tests still require that "arbitrary" warning flag be enabled.

I don't understand? The only reason these tests were using that flag was to cause a compilation error (instead of a warning) on GCC. If we properly require that we're able to check the diagnostics, which is the goal, we can stop trying to make them fail arbitrarily on GCC.

Why do we need separate options for each project?
I think that's the wrong direction.

These tests still require that "arbitrary" warning flag be enabled.

I'm not sure I love this, but I'm OK demoting warnings to errors in the XFAIL tests.

In D76150#1922104, @dim wrote:

Note: one thing that I am unsure about is the _NOEXCEPT after some of the copy constructors. I only applied those to classes that have other _NOEXCEPT constructors, but copying may involve other things that could throw.

Maybe it is safer to not use _NOEXCEPT at all for all of them, instead?

Could we add a .fail.cpp test that ensures when _LIBCPP_HAS_NO_FGETPOS is defined we don't actually have it?

Some of the changes in this patch are not correct. Please revert them in a follow up commit.

I think there is a bug in the paper, but I think the return type should be difference_type not size_type.
The paper says the return value is equivalent to:

LGTM other than the lack of tests.

Does this really need concepts?

Could you please upload this diff with more context?
(https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface)

In D75795#1918353, @rprichard wrote:

Sorry about this -- I'll try to be more careful in the future about getting my changes vetted.

@rprichard has no commits to libc++/libc++abi

Pedantic: I do have a couple libcxxabi patches, but @EricWF committed them for me (D36446 and D36447).

Why are we declaring this function at all?

I don't know. __cxa_finalize and __cxa_atexit are part of the IA-64 C++ ABI, so I suppose the notice at the top of the file applies. They're implemented elsewhere, though, not in libcxxabi.

/*
 * This header provides the interface to the C++ ABI as defined at:
 *       https://itanium-cxx-abi.github.io/cxx-abi/
 */

Is this just a clean move, or is the class definition changed as well?
I can't tell with all the linter warnings in the way.

Your good to commit.

• This check should suggest fixes. It knows what function is actually being resolved, and it has enough information to create a minimally qualified name that resolves it.
• This check should ignore hidden friends, since their entire purpose is to be called via ADL.
• This check should allow whitelisting namespaces that opt-out of ADL into their namespace. This makes it much easier to roll out the clang-tidy incrementally.

I'll be picking this up seriously this week.
I'm currently getting an internal version of this clang-tidy reviewed,
and my plan was to submit it upstream after because I didn't see this
patch until now (bad email filters).

I've already stated my disapproval of this patch. Libc++ has never and will never provide nor value C++03 conformance.
Moving backwards to C++03 is inconsistent with the libraries general direction.

Why are we declaring this function at all?

Why was this file ever getting built as plain C anyway?
All of the libc++abi source files that use it should be C++.

Abandoning. We went in another direction.