<div><div dir="auto">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.</div><br><div dir="auto">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.</div><div dir="auto"><br></div><div dir="auto">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.</div><div dir="auto"><br></div><div dir="auto">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.</div></div><div dir="auto"><br></div><div><div class="gmail_quote"><div>John.</div><div dir="auto"><br></div><div dir="auto">Eric Fiselier via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>> wrote:</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">EricWF added inline comments.<br>
<br>
<br>
================<br>
Comment at: lib/Sema/SemaDeclCXX.cpp:8931<br>
+                          /*ConstArg*/ true, false, false, false, false);<br>
+  auto *CopyCtor = cast_or_null<CXXConstructorDecl>(SMI.getMethod());<br>
+<br>
----------------<br>
rjmccall wrote:<br>
> Sorry, I didn't realize you'd go off and write this code manually.<br>
> <br>
> 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.<br>
I think this goes against the direction Richard and I decided to go, which was to not synthesize any expressions.<br>
<br>
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.<br>
<br>
Although the checking here leaves something to be desired, I suspect it's more than enough to handle any conforming STL implementation.<br>
<br>
<br>
<br>
<br>
<a href="https://reviews.llvm.org/D45476" rel="noreferrer" target="_blank">https://reviews.llvm.org/D45476</a><br>
<br>
<br>
<br>
</blockquote></div></div>