type trait builtins __remove_cv[ref] are used unconditionally

[AndreyG]

After the commit 5535716 by @philnik777 the usages of builtin type traits __remove_cv/__remove_cvref are not guarded by #if __has_builtin(__remove_cv[ref]):

llvm-project/libcxx/include/__type_traits/remove_cv.h

Line 22 in da0c8b2

using type _LIBCPP_NODEBUG = __remove_cv(_Tp);

llvm-project/libcxx/include/__type_traits/remove_cvref.h

Line 42 in da0c8b2

using type _LIBCPP_NODEBUG = __remove_cvref(_Tp);

Note that even the remaining comment is wrong now #endif // __has_builtin(__remove_cv).

llvm-project/libcxx/include/__type_traits/remove_cv.h

Line 31 in da0c8b2

#endif // __has_builtin(__remove_cv)

Also note that all other type traits builtins are used only after the check, for instance __add_pointer here:

llvm-project/libcxx/include/__type_traits/add_pointer.h

Line 23 in da0c8b2

#if !defined(_LIBCPP_WORKAROUND_OBJCXX_COMPILER_INTRINSICS) && __has_builtin(__add_pointer)

.

[philnik777]

What exactly is your point/question/bug report?

Also note that all other type traits builtins are used only after the check

That's simply not true. Lots of type traits don't have guards. Only builtin traits which aren't supported by all supported compilers have guards.

[AndreyG]

What exactly is your point/question/bug report?

I'd like to have usages of builtin type traits guarded. The motivation for this is the homegrown language engine which works pretty well with libc++ (and any C++ code in general) but doesn't support these type traits yet.

[philnik777]

I'm not sure what exactly you're referring to, but we can't support arbitrary C++ parsers. From our side the patch simply removed dead code. You can ask for official support for your tool, but without pre-commit testing that most likely won't happen.

[AndreyG]

Sorry, but I don't understand what's the problem with providing the fallback for parsers that don't support the builtin, how it was before the commit 5535716. This is also how a lot of type traits are implemented right now:
add_lvalue_reference
add_rvalue_reference
add_pointer
decay
remove_all_extents
remove_const
remove_extent
remove_pointer
remove_reference
remove_volatile
After all, that's the whole point of having the ability to check __has_builtin(...), right?

[philnik777]

There are also lots of traits which don't check the flag. We don't support any compilers where __has_builtin(__remove_cv) is false, so it's simply dead code. We don't need to check a condition which is always true. We can't keep every workaround forever, that's unmaintainable. The type traits you list all don't have corresponding builtins in GCC currently, so the #else implementation is checked and used with GCC.

[AndreyG]

We don't support any compilers where __has_builtin(__remove_cv) is false

It contradicts the very first statement phrase from the official documentation: "Libc++ aims to support common compilers that implement the C++11 Standard."

[philnik777]

And the second sentence states "In order to strike a good balance between stability for users and maintenance cost, testing coverage and development velocity, libc++ drops support for older compilers as newer ones are released.". Don't cherry-pick your quotes.

[ldionne]

We don't support any compilers where __has_builtin(__remove_cv) is false

It contradicts the very first statement phrase from the official documentation: "Libc++ aims to support common compilers that implement the C++11 Standard."

Most importantly, we do in fact support common compilers, but we don't support arbitrary ones. We support GCC, Clang and most Clang-based compilers (AppleClang, IBM's compiler, etc). But no, we don't support arbitrary compilers or arbitrarily old versions of the above compilers because that would not be viable maintenance-wise. We also don't aim to support arbitrary homegrown language engines.

I don't mean this in a hostile way -- it would be great for your tool to work with libc++, however we unfortunately can't start taking into account these kinds of niche requests for support because that would never end. Libc++ is used extremely widely and you'd be surprised what kind of stuff people want to do with it, and what kind of requests we get. In order for the project to stay relevant, it has to stay focused, and that often means saying no.

[Smertig]

Hey there. For the context: I'm @AndreyG's colleague.

In order to strike a good balance between stability for users and maintenance cost, testing coverage and development velocity, libc++ drops support for older compilers as newer ones are released.

Thanks for pointing out; it makes total sense. I fully understand that supporting custom/old compilers doesn't fit in the libc++ development process (and there's no way to check compatibility with custom tools).

However, may I ask you for the temporary exclusion here and providing fallback implementation for 3-6 more months until tooling is fully ready for those changes? Specifically, I'm talking about language support in CLion IDE / ReSharper C++ / Rider (three JetBrains products).

We are ready with internal implementation, but delivering it to end users might take some time (you know, releasing processes). I'm asking because we already have reports from users who tried fresh libc++ in their projects (i.e. chromium) and got a lot of red code in their editor even with basic stuff:

This was indeed our fault that we didn't provide implementation of those type traits in time, but that approach worked fine recently: usually, type traits were guarded, and we had some time to adapt. Now we realize that we should not rely on guards anymore and will support new builtin traits as fast as possible.

I really hope that suggested changes won't affect the development process of libc++ too much. I'm ready to create corresponding PR, if needed.

Thanks!

[philnik777]

@Smertig If you want to revert IMO we should have some sort of test coverage for your tools. That could be as simple as running the parser over all headers and making sure it works. I realize that that's a request which won't be fulfilled in a few days, so I'd be happy with a promise that you'll work with us to set something up. That gives us visibility into any problems your parser has with our code, and you get very early reports when anything breaks.

[ldionne]

@philnik777 I would be fine with reverting that commit from release/19.x just to give folks more time as a one-off thing. The patch is small and if we only reverted it from release/19.x, it's as-if we didn't change anything from the maintenance point of view since we don't revert anything in main. WDYT?

[philnik777]

@ldionne sure, we can do that. I'd be against reverting on trunk though. Given that chromium was mentioned it seemed to me like reverting trunk was also requested.

[ldionne]

@nico Can you chime in?

FWIW I'd be against reverting on main as well, at least that's my first reaction.

[Smertig]

If you want to revert IMO we should have some sort of test coverage for your tools

That's a good point. Unfortunately, I don't have a good solution that can be added to libc++'s CI right now, but I'll investigate possible options taking into account all the licensing factors.

As a first step, we'll try to run our tooling over main libc++ branch daily (on our side) and support all the breaking changes ASAP. I believe, this way we would be able to avoid red code for released libc++ version (but not for trunk, unfortunately).

I would be fine with reverting that commit from release/19.x just to give folks more time as a one-off thing. The patch is small and if we only reverted it from release/19.x, it's as-if we didn't change anything from the maintenance point of view since we don't revert anything in main.

It would be really nice, thank you! I also understand that reverting something from main doesn't sound like good idea (commit can be just lost), so I won't insist on doing that. However, this could potentially help a small number of our users who prefer trunk (i.e. chromium developers).