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

[John McCall via cfe-commits]

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. 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.
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.

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.

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.

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.  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.

John.

Eric Fiselier via Phabricator <reviews at reviews.llvm.org> wrote:

> EricWF added inline comments.
>
>
> ================
> Comment at: lib/Sema/SemaDeclCXX.cpp:8931
> +                          /*ConstArg*/ true, false, false, false, false);
> +  auto *CopyCtor = cast_or_null<CXXConstructorDecl>(SMI.getMethod());
> +
> ----------------
> rjmccall wrote:
> > Sorry, I didn't realize you'd go off and write this code manually.
> >
> > The way we generally handle this sort of thing is just to synthesize an
> expression in Sema that performs the copy-construction.  That way, the
> stdlib can be as crazy as it wants — default arguments on the
> copy-constructor or whatever else — and it just works.  The pattern to
> follow here is the code in Sema::BuildExceptionDeclaration, except that in
> your case you can completely dispense with faking up an OpaqueValueExpr and
> instead just build a DeclRefExpr to the actual variable.  (You could even
> use ActOnIdExpression for this, although just synthesizing the DRE
> shouldn't be too hard.)  Then the ComparisonCategoryInfo can just store
> expressions for each of the three (four?) variables, and in IRGen you just
> evaluate the appropriate one into the destination.
> I think this goes against the direction Richard and I decided to go, which
> was to not synthesize any expressions.
>
> The problem is that the synthesized expressions cannot be stored in
> `ComparisonCategoryInfo`, because the data it contains is not serialized.
> So when we read the AST back, the `ComparisonCategoryInfo` will be empty.
> And for obvious reasons we can't cache the data in Sema either.
> Additionally, we probably don't want to waste space building and storing
> synthesized expressions in each AST node which needs them.
>
> Although the checking here leaves something to be desired, I suspect it's
> more than enough to handle any conforming STL implementation.
>
>
>
>
> https://reviews.llvm.org/D45476
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180504/396857d2/attachment-0001.html>