[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In https://reviews.llvm.org/D45476#1087487, @EricWF wrote:

> In https://reviews.llvm.org/D45476#1087446, @cfe-commits wrote:
>
> > I think you and Richard agreed that you weren’t going to synthesize a whole
> >  expression tree at every use of the operator, and I agree with that
> >  decision. That’s very different from what I’m asking you to do, which is to
> >  synthesize in isolation a call to the copy-constructor.
>
>
> Perhaps. My apologies. I'm still quite new to the Clang internals. I appreciate your patience.
>
> > There are several places in the compiler that require these implicit copies which aren’t just
> >  normal expressions; this is the common pattern for handling them. The
> >  synthesized expression can be emitted multiple times, and it can be freely
> >  re-synthesized in different translation units instead of being serialized.
>
> I'm not sure this is always the case. For example:
>
>   // foo.h -- compiled as module.
>   #include <compare>
>   struct Foo { int x; };
>   inline auto operator<=>(Foo const& LHS, Foo const& RHS) {
>     // CXXConstructExpr's would be built when initially building the expression
>     // below. But the caches in ASTContext would not be serialized.
>     return LHS.x <=> RHS.x;
>   }
>   // foo.cpp
>   #include <foo.h> // imported via module.
>   auto bar(Foo LHS, Foo RHS) {
>     // The expression below calls a user defined comparison operator,
>     // so Sema doesn't process any of the types in <compare>, but it
>     // does generate code for the inline function, which requires <compare>;
>     // but it's too late to synthesize a CXXConstructExpr. 
>     return LHS <=> RHS;
>   }
>
>
>
>
> > You’re already finding and caching a constructor; storing a
> >  CXXConstructExpr is basically thr same thing, but in a nicer form that
> >  handles more cases and doesn’t require as much redundant code in IRGen.
>
> I'm not actually caching the copy constructor. And I disagree that storing a
>  `CXXConstructExpr` is essentially the same thing. I can lookup the `CXXConstructorDecl` without `Sema`,
>  but I can't build a `CXXConstructExpr` without it.


Ah.  If you want to be able to find this thing without a Sema around in order to
handle deserialized expressions by just caching things in the ASTContext, yes,
I agree that it would be difficult to build a `CXXConstructExpr` correctly.  I don't
fully understand the goal of avoiding serialization here, though.

>> STLs *frequently* make use of default arguments on copy constructors (for
>>  allocators). I agree that it’s unlikely that that would happen here, but
>>  that’s precisely because it’s unlikely that this type would ever be
>>  non-trivial.
> 
> A couple of points. First, when I say copy constructor, I mean the special member, which
>  cannot have default arguments,

I'm sorry, but this is just not true.  The formation rules for a copy constructor are laid
out in [class.copy]p2, and they explicitly allow default arguments.

> Also note that it's always the case that at least one copy constructor participates
>  in overload resolution.

But it's not true that that copy constructor has to be selected by overload resolution.

> Second, in the synopsis for the STL types, no constructors are declared. Although I don't
>  think the standard spells it out anywhere, I believe this forbids the types from having user
>  defined constructors (though perhaps not user-declared explicitly defaulted constructors.
>  For example adding a user defined destructor would be non-conforming since it makes
>  the types non-literal).

That would certainly be helpful, but I don't think it's true; in general the standard describes
what things are guaranteed to work with library types, but it doesn't generally constrain
the existence of other operations.

> Third, the STL spec implicitly requires the comparison category types be `CopyConstructible`
>  (The comparison operators are pass by value, and the valid values are declared as const).

Yes, of course.

> Barring STL maintainers that are out of their mind, I posit that the copy constructor
>  `T(T const&)` will always be available.  However, the STL types are free to add base
>  classes and fields arbitrarily. I could imagine some weird reasons why STL's might
>  want to have non-trivial members or bases.

I think it is reasonable to assume that these types will always be copy-constructible
from a const l-value, but as mentioned above, that doesn't mean the copy-constructor
has to be declared as `T(T const &)`.

>> Mostly, though, I don’t understand the point of imposing a partial set of
>>  non-conformant restrictions on the type. It’s easy to support an arbitrary
>>  copy constructor by synthesizing a CXXConstructExpr, and this will
>>  magically take care of any constexpr issues, as well as removing the need
>>  for open-coding a constructor call.
> 
> Fully checking the validity of copy construction would be preferable. I'll
>  attempting to implement what you're suggesting, and work past the
>  problematic example given earlier.

Okay, thanks.

>> The constexpr issues are that certain references to constexpr variables of
>>  literal type (as these types are likely to be) are required to not ODR-use
>>  the variable but instead just directly produce the initializer as the
>>  expression result.
> 
> Ah, I see. So the standard already requires the category types to be literal.
>  So we can always avoid ODR use.
> 
>> That’s especially important here because (1) existing
>>  STL binaries will not define these variables, and we shouldn’t create
>>  artificial deployment problems for this feature, and (2) we’d really rather
>>  not emit these expressions as loads from externally-defined variables that
>>  the optimizer won’t be able to optimize.
> 
> The variables are declared as `inline` and `constexpr`. There won't be binary compatibility
>  issues nor problems with opaque definitions.

Ah, right, I'd forgotten that C++ finally added `inline` variables.  Yes, that would solve
the opaqueness and compatibility problems.

John.


https://reviews.llvm.org/D45476