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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: clang/lib/Frontend/InitPreprocessor.cpp:677
     //Builder.defineMacro("__cpp_aggregate_paren_init", "201902L");
-    Builder.defineMacro("__cpp_concepts", "201907L");
+    Builder.defineMacro("__cpp_concepts", "202002L");
     Builder.defineMacro("__cpp_conditional_explicit", "201806L");
----------------
royjacobson wrote:
> erichkeane wrote:
> > aaron.ballman wrote:
> > > cor3ntin wrote:
> > > > royjacobson wrote:
> > > > > aaron.ballman wrote:
> > > > > > 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.)
> > > > > 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.
> > > > > 
> > > > 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.
> > > > 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.)
> > 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.
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128619/new/

https://reviews.llvm.org/D128619