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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
