[Clang] Implement P0848 (Conditionally Trivial Special Member Functions)

[royjacobson]

Summary

This patch implements P0848 in Clang.

During the instantiation of a C++ class, in Sema::ActOnFields, we evaluate constraints for all the SMFs and compare the constraints to compute the eligibility. We defer the computation of the type's [copy-]trivial bits from addedMember to the eligibility computation, like we did for destructors in D126194. canPassInRegisters is modified as well to better respect the ineligibility of functions.

Note: Because of the non-implementation of DR1734 and DR1496, I treat deleted member functions as 'eligible' for the purpose of [copy-]triviallity. This is unfortunate, but I couldn't think of a way to make this make sense otherwise.

[cor3ntin]

clang/lib/Sema/SemaDecl.cpp

18074

I'm arguing for checking the constraints at the completion of Base, and for making is_trivial_v/sizeof ill formed in this context.

Your GCC example is a bit misleading, I think. They check the sizeof(T) > 0 constraint at the completion of Base but they just swallow the error and declare the constraint unsatisfied or something. They should've probably issued a diagnostic or something. But if you look at which constructor they choose, they choose the unconstrained one: https://godbolt.org/z/rKj4q5Yx9

I don't think gcc is wrong per http://eel.is/c++draft/temp.constr.constr#temp.constr.op-5.sentence-3
The constrained overload is sfinea away silently, no diagnostic should be emitted.
It's no different from https://godbolt.org/z/ThMPrhs93 , for example

[royjacobson]

Add a test for the CRTP constraints timings

[royjacobson]

clang/lib/Sema/SemaDecl.cpp

18074

I added the test, could you take a look and tell me if that's what you meant or if you want some changes? @erichkeane
I hope it isn't disruptive to the deferred patch itself, it's a bit concerning indeed but I'm moderately optimistic :)

@cor3ntin thanks for the explanation!

[cor3ntin]

clang/test/SemaCXX/constrained-special-member-functions.cpp

104

Have you rebased since D127593 landed?

[royjacobson]

clang/test/SemaCXX/constrained-special-member-functions.cpp

104

Yes. It's related but it's different DRs. And unfortunately they seem much more ABI breaking to fix - GCC don't implement them, for example

[shafik]

clang/lib/AST/DeclCXX.cpp

899

I think we should spell out Special Member Function instead of using SMF

clang/lib/Sema/SemaDecl.cpp

18050

What happens if we have further parameters with default arguments? Unless I am missing something they are still special member functions but the proposal does not seem to cover them.

[royjacobson]

clang/lib/Sema/SemaDecl.cpp

18050

That's an excellent question.

I'm not sure what to do about default arguments. In a context where the additional parameters matter, you're not using them as constructors anymore, right? So why would this affect the type traits?
On the one hand [over.match.best] is against this idea of comparing constraints when the parameters differ. So also every context where this actually matters the overload resolution would probably be ambiguous anyway?

@BRevzin, what do you think? Is the wording intentional to include copy/move constructors with default arguments as well?

I checked with GCC and they seem to handle default arguments separately: https://godbolt.org/z/1ch3M7MjP

[royjacobson]

Rebase on main and slightly update docs.

[royjacobson]

@erichkeane @cor3ntin

If you have time for review right now, I think it's reasonably ready and I would still like to merge it this week so it can land in Clang15.

[royjacobson]

Rebase on main and re-target LLVM16 in cxx_status

[royjacobson]

Rebase

[royjacobson]

clang/docs/ReleaseNotes.rst

157–176

I'm not very opinionated about this, but I tend to not significant enough. I mean, 7 years later and only MSVC have even bothered to implement them.

clang/lib/Sema/SemaDecl.cpp

18050

FWIW, I filed https://github.com/cplusplus/CWG/issues/110

I'm not on the reflector myself, so I don't know if there's been a follow-up there.

[usaxena95]

I would like to see this land. This also fixes https://github.com/llvm/llvm-project/issues/57046.

Are there any pending concerns of the reviewers which we need to address ? Happy to help in any way to move this patch forward.

CC: @erichkeane @cor3ntin

[cor3ntin]

This looks good to me.

I *think* landing things without doing silent sfinae for incomplete types is okay-ish, but I'd like to here from other folks.

@royjacobson Have you looked into that? Do you know how involved would it be?

Either way, we should make sure that if that bit is dealt with separately, we have an issue or something to track it, maybe a note in cxx_status.

clang/docs/ReleaseNotes.rst

157–176

We might has well, I think it's a good way to not lose track of it.

clang/lib/Sema/SemaDecl.cpp

18050

Yes, i think it's reasonable to not concerns ourselves with default parameters until wg21 decides otherwise

clang/test/SemaCXX/constrained-special-member-functions.cpp

201

Have you been able to investigate that?

[royjacobson]

Add disclaimer to cxx_status about the standing DRs.

[royjacobson]

clang/docs/ReleaseNotes.rst

157–176

Added as a footnote.

clang/test/SemaCXX/constrained-special-member-functions.cpp

201

No, I haven't looked at this - we already have a bunch of those all over the code, see https://github.com/llvm/llvm-project/issues/48857

So I don't think this is about this patch specifically - I just call Sema::CheckFunctionConstraints and inherit its current behavior.

[cor3ntin]

@aaron.ballman I think this looks good, you want to double check?

[aaron.ballman]

clang/lib/AST/DeclCXX.cpp

1443–1447

clang/lib/Frontend/InitPreprocessor.cpp

681

Does any of the not-yet-implemented bits (including from the DRs) impact the ability to use conditionally trivial special member functions? If so, we might want to be careful about aggressively bumping this value. (It's more palatable for us to come back and bump the value later than it is for us to claim we implement something fully when we know we don't -- the goal of the feature test macros is so that users don't have to resort to compiler version checks, which is what users have to use when they fall into that "not fully implemented" space.)

clang/lib/Sema/SemaDecl.cpp

18045

Yeah, we prefer putting types into an anonymous namespace but making functions static: https://llvm.org/docs/CodingStandards.html#anonymous-namespaces

18065

You may want to run the patch through clang-format before you land, just to be sure (https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting).

clang/test/AST/conditionally-trivial-smfs.cpp

344

Please add a newline to the end of the file.

clang/www/cxx_status.html

929–930

FWIW, the way we've started handling this in recent history is to use "partial" and a details tag instead of a footnote, as in: https://github.com/llvm/llvm-project/blob/main/clang/www/cxx_status.html#L915.

[usaxena95]

Can you please also add consteval related test case (mentioned in https://github.com/llvm/llvm-project/issues/57046#issuecomment-1211665079) to cxx2a-consteval.cpp

[royjacobson]

Address Aaron's comments, add the consteval test.

[royjacobson]

clang/lib/Frontend/InitPreprocessor.cpp

681

I don't think they're very significant, and the benefits of enabling it seem large enough for me - for example, std::expected works with libstdc++ and passes their unit tests but is gated by this macro.

We still have a non-trivial amount of concept bugs to go over, but I support enabling this.

clang/www/cxx_status.html

929–930

It felt a bit too long of an explanation to put in the tiny table box, but I don't feel very strongly about it either way.

[cor3ntin]

clang/lib/Frontend/InitPreprocessor.cpp

681

I think it's better to be conservative, It's the lesser of two not great options.
I'm hoping we can get to fix the issues in the clang 16 cycle , but in the meantime we should not claim conformance if we are not, in fact, conforming.

clang/www/cxx_status.html

929–930

I agree with Aaron.
I think it's better to be consistent, the column resize when the details are expanded.

[aaron.ballman]

clang/lib/Frontend/InitPreprocessor.cpp

681

We still have a non-trivial amount of concept bugs to go over, but I support enabling this.

I think we should specify the updated macro value only after we think concepts have no known major issues with them. (Unknown issues happen and there's not much we can do about them, this is more that we shouldn't specify that we conform to a particular feature test macro when we knowingly still don't have it right yet.)

[erichkeane]

clang/lib/Frontend/InitPreprocessor.cpp

681

Yeah, I don't think we can set this until we can at least compile a basic libstdc++ ranges use, which the deferred instantiation still holds up. That would be my 'bare minimum' test for whether we can set that.

[royjacobson]

clang/lib/Frontend/InitPreprocessor.cpp

681

But we're already defining the __cpp_concepts macro, even with the current known bugs. The version bump, introduced by https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p2493r0.html is specifically about the conditionally trivial SMFs paper.

We can decide that we want the version bump to mean 'no more known concept bugs in clang' instead. But that would extend the meaning of the macro and would be confusing to users who want to rely on the documented, intended meaning of the version.

Also I think telling library writers 'this feature works now, but we didn't set its feature test macro' will make them use more compiler version checks, not less.

[erichkeane]

clang/lib/Frontend/InitPreprocessor.cpp

681

The feature test macros aren't supposed to mean "I support the feature from the paper that updated it". They are intended to mean "I support the feature from the paper that updated it AND everything before it".

I don't believe we need to be bug-free, but something as extreme as "we can't compile a large number of uses of concepts because we don't implement a central design consideration" (which, btw, was clarified in Core after the 2019 date IIRC) means we shouldn't be updating this.

[royjacobson]

Remove the __cpp_concepts version bump.

[royjacobson]

clang/lib/Frontend/InitPreprocessor.cpp

681

It seems like I'm in the minority here, so I removed the version change and added a comment with a reference to this discussion to the code.

[aaron.ballman]

clang/lib/Frontend/InitPreprocessor.cpp

681

But we're already defining the __cpp_concepts macro, even with the current known bugs.

FWIW, I consider that a bug. We should have never defined this macro in the first place -- we're not there yet on the base feature.

I don't believe we need to be bug-free, but something as extreme as "we can't compile a large number of uses of concepts because we don't implement a central design consideration" (which, btw, was clarified in Core after the 2019 date IIRC) means we shouldn't be updating this.

+1

[royjacobson]

Clarify ambiguous doc.

[cor3ntin]

I would wait for @aaron.ballman or @erichkeane to sign off on it, but this looks good to me.
Thanks again for working on this!

[aaron.ballman]

LGTM, thank you for this!!

[royjacobson]

Rebase on main.

[haowei]

Hi,

We are seeing build failures on Fuchsia build bots after this change was landed. and we suspect this patch causes clang to miscompile certain code. I filed bug https://github.com/llvm/llvm-project/issues/57351 and included a reproducer. Could you take a look? If it is confirmed to be a bug and take long time to fix, could you revert this change please?

[royjacobson]

Default constructors can be template functions. Update patch to handle them correctly, and add
asserts to LookupSpecialMember that can catch similar problems.

[royjacobson]

I added handling of FunctionTemplateDecls in SemaDecl because they can be default constructors. This caused errors with std::pair for example - it has two enable_ifs constructors to control explicit/implicit construction.

I will try committing again if the pre-commit CI finishes green. Running nm on the provided reproducer from FuchsiaOS resulted in the names list.

[cor3ntin]

clang/test/AST/conditionally-trivial-smfs-1.cpp

1 ↗

(On Diff #455686)

can you rename that file back to clang/test/AST/conditionally-trivial-smfs.cpp ? Thanks!

[royjacobson]

Rename file

[royjacobson]

We call LookupSpecialMember before we compute eligibility, so the assert fails. Remove it.

[royjacobson]

Formatting.