Implicit move constructors in clang

[chandlerc, 2011-06-23 06:07:09]

http://codereview.appspot.com/4630058/diff/1011/include/clang/AST/DeclCXX.h
File include/clang/AST/DeclCXX.h (right):

http://codereview.appspot.com/4630058/diff/1011/include/clang/AST/DeclCXX.h#n...
include/clang/AST/DeclCXX.h:297: /// (probably) have an implicit move
constructor declared for it.
Probably? I don't that's a reasonable interface description. ;]

"Reading this variable probably won't cause demons to fly out of your nose."

Also, I'm skeptical about the need for new state bits for these. Reading the
implementation, it seems like you could define an interface for querying whether
an implicit move constructor was needed which would simply be a logical
combination of existing bits. Same for MoveAssignment below.

http://codereview.appspot.com/4630058/diff/1011/include/clang/AST/DeclCXX.h#n...
include/clang/AST/DeclCXX.h:714: return
getASTContext().getLangOptions().CPlusPlus0x &&
It would seem a lot cheaper to only set the bit when in CPlusPlus0x mode than to
query this on every call...

http://codereview.appspot.com/4630058/diff/1011/include/clang/AST/DeclCXX.h#n...
include/clang/AST/DeclCXX.h:717: void setNeedsImplicitMoveConstructor(bool B) {
Why setters? The AST is moving away from setters entirely.

http://codereview.appspot.com/4630058/diff/1011/include/clang/Sema/Sema.h
File include/clang/Sema/Sema.h (right):

http://codereview.appspot.com/4630058/diff/1011/include/clang/Sema/Sema.h#new...
include/clang/Sema/Sema.h:2811: CXXConstructorDecl
*MaybeDeclareImplicitMoveConstructor(
I don't like "maybe" in interfaces. It leaves the caller wondering under what
circumstances the action doesn't take place.

Generally I'd rather just "DeclareImplicitMoveConstructor" and document
thoroughly the behavior in the case that it would be defined as deleted. Also,
potentially provide an optional bool output parameter (or enum-based multi-state
return) so that a caller which wishes to distinguish between the cases can?

http://codereview.appspot.com/4630058/diff/1011/lib/AST/DeclCXX.cpp
File lib/AST/DeclCXX.cpp (right):

http://codereview.appspot.com/4630058/diff/1011/lib/AST/DeclCXX.cpp#newcode34
lib/AST/DeclCXX.cpp:34: NeedsImplicitMoveConstructor(true),
NeedsImplicitMoveAssignment(false),
I find these initial values rather surprising. It would be good to document them
somewhere, potentially in the doxyments for these bits.

http://codereview.appspot.com/4630058/diff/1011/lib/Sema/SemaDeclCXX.cpp
File lib/Sema/SemaDeclCXX.cpp (right):

http://codereview.appspot.com/4630058/diff/1011/lib/Sema/SemaDeclCXX.cpp#newc...
lib/Sema/SemaDeclCXX.cpp:3746: /// involving dependent friends where this
/might/ not be true, but probably
Rather than "probably not" I would say more explicitly that we don't model
dependent friends in that case. Maybe even why, or in what ways it can fail.
Comments cost little (unlike code) and may help a future reader.

http://codereview.appspot.com/4630058/diff/1011/lib/Sema/SemaDeclCXX.cpp#newc...
lib/Sema/SemaDeclCXX.cpp:3748: bool
Sema::ShouldDeleteMoveConstructor(CXXRecordDecl *RD, unsigned ArgQuals) {
This method is enormous. I think it would be much easier to read if you factored
it into a few static helpers. It seems like there is a fair amount of
repeatition as well.

Actually, after reading this method in detail, it really doesn't seem like the
correct approach. There is a consistent and fairly pervasive pattern to compute
these properties of CXXRecordDecls as we build it up. That's how all the other
bits of state are managed. I don't see anything fundamental about the algorithm
you have implemented here that precludes that. Doing so would remove the cost of
querying for whether the constructor must be deleted, which would tremendously
simplify much below....

http://codereview.appspot.com/4630058/diff/1011/lib/Sema/SemaDeclCXX.cpp#newc...
lib/Sema/SemaDeclCXX.cpp:3759: if (Union && RD->isAnonymousStructOrUnion())
The comment makes this sound like it should either be an assert or a diagnostic.

http://codereview.appspot.com/4630058/diff/1011/lib/Sema/SemaDeclCXX.cpp#newc...
lib/Sema/SemaDeclCXX.cpp:3765: //    A defaulted [move] constructor for class X
is defined as delete if X has:
80 columns.

http://codereview.appspot.com/4630058/diff/1011/lib/Sema/SemaDeclCXX.cpp#newc...
lib/Sema/SemaDeclCXX.cpp:3770: // We'll handle this one later
Why? It's much more expensive to iterate twice...

http://codereview.appspot.com/4630058/diff/1011/lib/Sema/SemaDeclCXX.cpp#newc...
lib/Sema/SemaDeclCXX.cpp:3814: // -- any [virtual base class] of a type with a
destructor that is deleted or
80 columns.

http://codereview.appspot.com/4630058/diff/1011/lib/Sema/SemaDeclCXX.cpp#newc...
lib/Sema/SemaDeclCXX.cpp:3853: // Anonymous unions inside unions do not variant
members create
Yoda speak?

http://codereview.appspot.com/4630058/diff/1011/lib/Sema/SemaDeclCXX.cpp#newc...
lib/Sema/SemaDeclCXX.cpp:7467: // Virtual bases are handled below.
Again, why? The code is *identical* in this case.

http://codereview.appspot.com/4630058/diff/1011/lib/Sema/SemaDeclCXX.cpp#newc...
lib/Sema/SemaDeclCXX.cpp:7505: "that class doesn't need an implicit move
constructor");
It seems quite strange that this is *maybe* declare implicit move constructor,
but it doesn't silently not do so when one is not needed.... but maybe this will
make more sense when the 'maybe' is removed from the interface.

http://codereview.appspot.com/4630058/diff/1011/lib/Sema/SemaDeclCXX.cpp#newc...
lib/Sema/SemaDeclCXX.cpp:7510:
ClassDecl->setNeedsImplicitMoveConstructor(false);
Yikes. This crazily violates the immutability of ASTs doesn't it? See my comment
above about my concern with the strategy for ShouldDeleteMoveConstructor. If we
fix that computation to occur while building the decl rather than on demand, we
can simply check that bit inside of needsImplicitMoveConstructor. No need for
mutation here.

http://codereview.appspot.com/4630058/diff/1011/test/SemaCXX/simple-implicit-...
File test/SemaCXX/simple-implicit-move.cpp (right):

http://codereview.appspot.com/4630058/diff/1011/test/SemaCXX/simple-implicit-...
test/SemaCXX/simple-implicit-move.cpp:1: // RUN: %clang_cc1 -fsyntax-only
-verify -std=c++0x %s
I'd not call this file 'simple-impliit-move.cpp'. Why not add tests under the
paragraphs of the draft standard being tested? move constructors seem a perfect
fit for the standard-based test suite tree.

[scshunt, 2011-06-23 06:28:07]

Responses to comments I thought were worth responding to by things other than
simple code edits.

http://codereview.appspot.com/4630058/diff/1011/include/clang/AST/DeclCXX.h
File include/clang/AST/DeclCXX.h (right):

http://codereview.appspot.com/4630058/diff/1011/include/clang/AST/DeclCXX.h#n...
include/clang/AST/DeclCXX.h:714: return
getASTContext().getLangOptions().CPlusPlus0x &&
On 2011/06/23 06:07:09, chandlerc wrote:
> It would seem a lot cheaper to only set the bit when in CPlusPlus0x mode than
to
> query this on every call...
Good catch

http://codereview.appspot.com/4630058/diff/1011/include/clang/AST/DeclCXX.h#n...
include/clang/AST/DeclCXX.h:717: void setNeedsImplicitMoveConstructor(bool B) {
On 2011/06/23 06:07:09, chandlerc wrote:
> Why setters? The AST is moving away from setters entirely.
Putting aside for the moment the fact that I've never heard that the AST is
moving away from setters, because we need to store this info somewhere, and
storing it on the record seems reasonable. If you'd prefer a different name for
the bit, that's fine, but it is not synthesizable from other bits due to the
fact that Sema can look at the class, say it would have a deleted move
constructor, and clear this bit.

This could go on Sema, but so could a number of other bits here too.

http://codereview.appspot.com/4630058/diff/1011/include/clang/Sema/Sema.h
File include/clang/Sema/Sema.h (right):

http://codereview.appspot.com/4630058/diff/1011/include/clang/Sema/Sema.h#new...
include/clang/Sema/Sema.h:2811: CXXConstructorDecl
*MaybeDeclareImplicitMoveConstructor(
On 2011/06/23 06:07:09, chandlerc wrote:
> I don't like "maybe" in interfaces. It leaves the caller wondering under what
> circumstances the action doesn't take place.
> 
> Generally I'd rather just "DeclareImplicitMoveConstructor" and document
> thoroughly the behavior in the case that it would be defined as deleted. Also,
> potentially provide an optional bool output parameter (or enum-based
multi-state
> return) so that a caller which wishes to distinguish between the cases can?

I'll add documentation that it can return 0, which is the failure case.

http://codereview.appspot.com/4630058/diff/1011/lib/Sema/SemaDeclCXX.cpp
File lib/Sema/SemaDeclCXX.cpp (right):

http://codereview.appspot.com/4630058/diff/1011/lib/Sema/SemaDeclCXX.cpp#newc...
lib/Sema/SemaDeclCXX.cpp:3746: /// involving dependent friends where this
/might/ not be true, but probably
On 2011/06/23 06:07:09, chandlerc wrote:
> Rather than "probably not" I would say more explicitly that we don't model
> dependent friends in that case. Maybe even why, or in what ways it can fail.
> Comments cost little (unlike code) and may help a future reader.
The probably refers to the slight uncertainty in the standard. I'm pretty sure
but not entirely convinced there isn't a magical corner case where this
assumption breaks.

http://codereview.appspot.com/4630058/diff/1011/lib/Sema/SemaDeclCXX.cpp#newc...
lib/Sema/SemaDeclCXX.cpp:3748: bool
Sema::ShouldDeleteMoveConstructor(CXXRecordDecl *RD, unsigned ArgQuals) {
On 2011/06/23 06:07:09, chandlerc wrote:
> This method is enormous. I think it would be much easier to read if you
factored
> it into a few static helpers. It seems like there is a fair amount of
> repeatition as well.
> 
> Actually, after reading this method in detail, it really doesn't seem like the
> correct approach. There is a consistent and fairly pervasive pattern to
compute
> these properties of CXXRecordDecls as we build it up. That's how all the other
> bits of state are managed. I don't see anything fundamental about the
algorithm
> you have implemented here that precludes that. Doing so would remove the cost
of
> querying for whether the constructor must be deleted, which would tremendously
> simplify much below....

This could be done for a few special cases, but not for the general case where
you could have four move constructors all explicitly declared and defaulted,
each being deleted for a different reason

http://codereview.appspot.com/4630058/diff/1011/lib/Sema/SemaDeclCXX.cpp#newc...
lib/Sema/SemaDeclCXX.cpp:3759: if (Union && RD->isAnonymousStructOrUnion())
On 2011/06/23 06:07:09, chandlerc wrote:
> The comment makes this sound like it should either be an assert or a
diagnostic.

I don't trust the code enough to put that in yet. The standard handles anonymous
enums in a very broken manner that must be ignored to get remotely sane
behavior; I have no clue where clang stands in that regard and they're uncommon
enough to not make it worth the investigation.

http://codereview.appspot.com/4630058/diff/1011/lib/Sema/SemaDeclCXX.cpp#newc...
lib/Sema/SemaDeclCXX.cpp:3770: // We'll handle this one later
On 2011/06/23 06:07:09, chandlerc wrote:
> Why? It's much more expensive to iterate twice...
Once over direct bases, once over virtual bases.

http://codereview.appspot.com/4630058/diff/1011/lib/Sema/SemaDeclCXX.cpp#newc...
lib/Sema/SemaDeclCXX.cpp:7467: // Virtual bases are handled below.
On 2011/06/23 06:07:09, chandlerc wrote:
> Again, why? The code is *identical* in this case.
Because we don't have a magical all_base_iterator. We probably should.

http://codereview.appspot.com/4630058/diff/1011/lib/Sema/SemaDeclCXX.cpp#newc...
lib/Sema/SemaDeclCXX.cpp:7510:
ClassDecl->setNeedsImplicitMoveConstructor(false);
On 2011/06/23 06:07:09, chandlerc wrote:
> Yikes. This crazily violates the immutability of ASTs doesn't it? See my
comment
> above about my concern with the strategy for ShouldDeleteMoveConstructor. If
we
> fix that computation to occur while building the decl rather than on demand,
we
> can simply check that bit inside of needsImplicitMoveConstructor. No need for
> mutation here.
Performing this computation when we build the record means greedily declaring
implicit copy constructors.

[chandlerc, 2011-06-23 06:46:31]

(just responding to a few of the comments that stood out to me)

http://codereview.appspot.com/4630058/diff/1011/lib/Sema/SemaDeclCXX.cpp
File lib/Sema/SemaDeclCXX.cpp (right):

http://codereview.appspot.com/4630058/diff/1011/lib/Sema/SemaDeclCXX.cpp#newc...
lib/Sema/SemaDeclCXX.cpp:3748: bool
Sema::ShouldDeleteMoveConstructor(CXXRecordDecl *RD, unsigned ArgQuals) {
On 2011/06/23 06:28:07, scshunt wrote:
> On 2011/06/23 06:07:09, chandlerc wrote:
> > This method is enormous. I think it would be much easier to read if you
> factored
> > it into a few static helpers. It seems like there is a fair amount of
> > repeatition as well.
> > 
> > Actually, after reading this method in detail, it really doesn't seem like
the
> > correct approach. There is a consistent and fairly pervasive pattern to
> compute
> > these properties of CXXRecordDecls as we build it up. That's how all the
other
> > bits of state are managed. I don't see anything fundamental about the
> algorithm
> > you have implemented here that precludes that. Doing so would remove the
cost
> of
> > querying for whether the constructor must be deleted, which would
tremendously
> > simplify much below....
> 
> This could be done for a few special cases, but not for the general case where
> you could have four move constructors all explicitly declared and defaulted,
> each being deleted for a different reason

Can you explain why? I can see no reason. This function does exactly four
things:

1) It checks the union (or non union) aspect of the record.
2) It iterates over each base, checking for a set of patterns which might cause
the constructor to be deleted.
3) It iterates over each virtual base, same as #2.
4) It iterates over each field, checking whether it fits a pattern which might
cause the constructor to be deleted.

#1 seems trivial to do when building the RecordDecl. 2, 3, and 4 each have a
method on CXXRecordDecl used to process the base class or field as it is added
to the record decl. I don't see anything in the base inspection that requires
all fields to be present in the current decl, or other order requirement that
would preclude this from working.

http://codereview.appspot.com/4630058/diff/1011/lib/Sema/SemaDeclCXX.cpp#newc...
lib/Sema/SemaDeclCXX.cpp:3759: if (Union && RD->isAnonymousStructOrUnion())
On 2011/06/23 06:28:07, scshunt wrote:
> On 2011/06/23 06:07:09, chandlerc wrote:
> > The comment makes this sound like it should either be an assert or a
> diagnostic.
> 
> I don't trust the code enough to put that in yet. The standard handles
anonymous
> enums in a very broken manner that must be ignored to get remotely sane
> behavior; I have no clue where clang stands in that regard and they're
uncommon
> enough to not make it worth the investigation.

The aren't that uncommon. We use them heavily. Even Clang and LLVM use them
heavily. I think this important to get right.

http://codereview.appspot.com/4630058/diff/1011/lib/Sema/SemaDeclCXX.cpp#newc...
lib/Sema/SemaDeclCXX.cpp:7467: // Virtual bases are handled below.
On 2011/06/23 06:28:07, scshunt wrote:
> On 2011/06/23 06:07:09, chandlerc wrote:
> > Again, why? The code is *identical* in this case.
> Because we don't have a magical all_base_iterator. We probably should.

I would expect either none of the virtual bases to be in this sequence, or all
of them to be in them. We should figure out which it is, and comment the
iterators on CXXRecordDecl appropriately.

I really thought that bases_begin() already was the 'all base iterator' you're
looking for. I'm able to trivially find numerous places in the code where it
claims to be collecting data from "all base classes" and only these are iterated
through.

http://codereview.appspot.com/4630058/diff/1011/lib/Sema/SemaDeclCXX.cpp#newc...
lib/Sema/SemaDeclCXX.cpp:7510:
ClassDecl->setNeedsImplicitMoveConstructor(false);
On 2011/06/23 06:28:07, scshunt wrote:
> On 2011/06/23 06:07:09, chandlerc wrote:
> > Yikes. This crazily violates the immutability of ASTs doesn't it? See my
> comment
> > above about my concern with the strategy for ShouldDeleteMoveConstructor. If
> we
> > fix that computation to occur while building the decl rather than on demand,
> we
> > can simply check that bit inside of needsImplicitMoveConstructor. No need
for
> > mutation here.
> Performing this computation when we build the record means greedily declaring
> implicit copy constructors.

I see, so your whole goal here is to defer declaring any implicit constructors
until we know we need this one, and then walk the type hierarchy declaring as we
need to...

I'm not really sure about this approach. It seems like we should simply compute
the necessary information to know whether or not deletion would occur (without
necessarily declaring anything, even if that involves "faking" the declaration)
from the bottom up as we parse code.

The two concerns I have with the lazy approach is that it harms locality, and it
degrades the effectiveness of PCH. Suddenly, we have to go recurse down through
declarations which may never be used elsewhere merely to compute the bit of
state about whether or not the move constructor would be deleted. That involves
deserialization etc.

I suspect we should talk the strategy here through with Doug and others to find
what the right performance / eager computation tradeoff is.

[DougGregor, 2011-06-24 16:18:35]

I think the only real issue in this patch is that we'd like to avoid adding the
NeedsMove* bits in CXXRecordDecl. If we can eliminate those, we'd be in great
shape.

That, and I think we should have a bit more test coverage for the various cases
where move constructors are/are not implicitly generated.

http://codereview.appspot.com/4630058/diff/1011/include/clang/AST/DeclCXX.h
File include/clang/AST/DeclCXX.h (right):

http://codereview.appspot.com/4630058/diff/1011/include/clang/AST/DeclCXX.h#n...
include/clang/AST/DeclCXX.h:297: /// (probably) have an implicit move
constructor declared for it.
On 2011/06/23 06:07:09, chandlerc wrote:
> Probably? I don't that's a reasonable interface description. ;]
> 
> "Reading this variable probably won't cause demons to fly out of your nose."
> 
> Also, I'm skeptical about the need for new state bits for these. Reading the
> implementation, it seems like you could define an interface for querying
whether
> an implicit move constructor was needed which would simply be a logical
> combination of existing bits. Same for MoveAssignment below.

The problem, I think, is the condition that a move constructor not be declared
if it would be defined as deleted. However, I'd still like to eliminate these
bits if possible.

http://codereview.appspot.com/4630058/diff/1011/include/clang/Sema/Sema.h
File include/clang/Sema/Sema.h (right):

http://codereview.appspot.com/4630058/diff/1011/include/clang/Sema/Sema.h#new...
include/clang/Sema/Sema.h:1691: CXXMethodDecl
*LookupCopyingAssignment(CXXRecordDecl *Class, unsigned Quals,
I know it's not unique to this patch, but I find the "ing" in
LookupMovingConstructor/LookupCopyingConstructor/LookupCopyingAssignment to be
weird.

http://codereview.appspot.com/4630058/diff/1011/include/clang/Sema/Sema.h#new...
include/clang/Sema/Sema.h:2721: 
You mean a defaulted *move* constructor in the comment.

http://codereview.appspot.com/4630058/diff/1011/lib/Sema/SemaDeclCXX.cpp
File lib/Sema/SemaDeclCXX.cpp (right):

http://codereview.appspot.com/4630058/diff/1011/lib/Sema/SemaDeclCXX.cpp#newc...
lib/Sema/SemaDeclCXX.cpp:3748: bool
Sema::ShouldDeleteMoveConstructor(CXXRecordDecl *RD, unsigned ArgQuals) {
On 2011/06/23 06:46:31, chandlerc wrote:
> On 2011/06/23 06:28:07, scshunt wrote:
> > On 2011/06/23 06:07:09, chandlerc wrote:
> > > This method is enormous. I think it would be much easier to read if you
> > factored
> > > it into a few static helpers. It seems like there is a fair amount of
> > > repeatition as well.
> > > 
> > > Actually, after reading this method in detail, it really doesn't seem like
> the
> > > correct approach. There is a consistent and fairly pervasive pattern to
> > compute
> > > these properties of CXXRecordDecls as we build it up. That's how all the
> other
> > > bits of state are managed. I don't see anything fundamental about the
> > algorithm
> > > you have implemented here that precludes that. Doing so would remove the
> cost
> > of
> > > querying for whether the constructor must be deleted, which would
> tremendously
> > > simplify much below....
> > 
> > This could be done for a few special cases, but not for the general case
where
> > you could have four move constructors all explicitly declared and defaulted,
> > each being deleted for a different reason
> 
> Can you explain why? I can see no reason. This function does exactly four
> things:
> 
> 1) It checks the union (or non union) aspect of the record.
> 2) It iterates over each base, checking for a set of patterns which might
cause
> the constructor to be deleted.
> 3) It iterates over each virtual base, same as #2.
> 4) It iterates over each field, checking whether it fits a pattern which might
> cause the constructor to be deleted.
> 
> #1 seems trivial to do when building the RecordDecl. 2, 3, and 4 each have a
> method on CXXRecordDecl used to process the base class or field as it is added
> to the record decl. I don't see anything in the base inspection that requires
> all fields to be present in the current decl, or other order requirement that
> would preclude this from working.

Access control is likely to be the big problem. We can't do access control from
inside the AST library.

http://codereview.appspot.com/4630058/diff/1011/lib/Sema/SemaDeclCXX.cpp#newc...
lib/Sema/SemaDeclCXX.cpp:7467: // Virtual bases are handled below.
On 2011/06/23 06:46:31, chandlerc wrote:
> On 2011/06/23 06:28:07, scshunt wrote:
> > On 2011/06/23 06:07:09, chandlerc wrote:
> > > Again, why? The code is *identical* in this case.
> > Because we don't have a magical all_base_iterator. We probably should.
> 
> I would expect either none of the virtual bases to be in this sequence, or all
> of them to be in them. We should figure out which it is, and comment the
> iterators on CXXRecordDecl appropriately.
> 
> I really thought that bases_begin() already was the 'all base iterator' you're
> looking for. I'm able to trivially find numerous places in the code where it
> claims to be collecting data from "all base classes" and only these are
iterated
> through.

We have two sequences: 
  bases_begin()/bases_end() contains all of the direct (virtual and non-virtual)
base classes. 

  vbases_begin()/vbases_end() contains all of the virtual base classes, both
direct and indirect.

It's fairly common in Sema to iterate over both sets. Perhaps we should abstract
this better with some kind of sequence-joining iterator, but Sean doesn't need
to do that in this patch.

http://codereview.appspot.com/4630058/diff/1011/lib/Sema/SemaDeclCXX.cpp#newc...
lib/Sema/SemaDeclCXX.cpp:7505: "that class doesn't need an implicit move
constructor");
On 2011/06/23 06:07:09, chandlerc wrote:
> It seems quite strange that this is *maybe* declare implicit move constructor,
> but it doesn't silently not do so when one is not needed.... but maybe this
will
> make more sense when the 'maybe' is removed from the interface.

DeclareImplicitMoveConstructorIfNotDeleted would be more accurate (albeit
verbose).

http://codereview.appspot.com/4630058/diff/1011/lib/Sema/SemaDeclCXX.cpp#newc...
lib/Sema/SemaDeclCXX.cpp:7510:
ClassDecl->setNeedsImplicitMoveConstructor(false);
On 2011/06/23 06:46:31, chandlerc wrote:
> On 2011/06/23 06:28:07, scshunt wrote:
> > On 2011/06/23 06:07:09, chandlerc wrote:
> > > Yikes. This crazily violates the immutability of ASTs doesn't it? See my
> > comment
> > > above about my concern with the strategy for ShouldDeleteMoveConstructor.
If
> > we
> > > fix that computation to occur while building the decl rather than on
demand,
> > we
> > > can simply check that bit inside of needsImplicitMoveConstructor. No need
> for
> > > mutation here.
> > Performing this computation when we build the record means greedily
declaring
> > implicit copy constructors.
> 
> I see, so your whole goal here is to defer declaring any implicit constructors
> until we know we need this one, and then walk the type hierarchy declaring as
we
> need to...
> 
> I'm not really sure about this approach. It seems like we should simply
compute
> the necessary information to know whether or not deletion would occur (without
> necessarily declaring anything, even if that involves "faking" the
declaration)
> from the bottom up as we parse code.
> 
> The two concerns I have with the lazy approach is that it harms locality, and
it
> degrades the effectiveness of PCH. Suddenly, we have to go recurse down
through
> declarations which may never be used elsewhere merely to compute the bit of
> state about whether or not the move constructor would be deleted. That
involves
> deserialization etc.
> 
> I suspect we should talk the strategy here through with Doug and others to
find
> what the right performance / eager computation tradeoff is.

We should lazily declare constructors as much as possible. I would expect this
to be handled by updating Sema::LookupConstructors, where we declare all of the
various implicitly-declared constructors at once (since most lookups look at all
of them anyway). It seems that we could keep the
NeedsImplicitMoveConstructor/NeedsImplicitMoveAssignment state local to that
routine and its subroutines, eliminating the need to have them in the AST. Is
that not possible for some reason, Sean?

Chandler is right that modifying the AST like this can cause PCH problems. We'd
likely need to call Decl::setChangedSinceDeserialization() to note that we've
updated bits, which causes all sorts of fun with update records in chained PCH.