[PATCH] D86660: Modifying ImportDeclContext(...) to ensure that we also handle the case when the FieldDecl is an ArrayType whose ElementType is a RecordDecl

[Pavel Labath via Phabricator via cfe-commits]

labath added a comment.

I'm going to respond to the rest of your (very insightful) comment later. So far, I'm just responding to this:

>>   This isn't exactly layout related, but there is the question of covariant methods. If a method is covariant, then its return type must be complete.
>
> We already import all the base classes of a derived class (at least in full import). But, we do not import all the derived classes during the import of the base. Is this what you do in LLDB to support covariant return types?

This situation is a bit tricky to explain as there are two independent class hierarchies that need to be considered. Let me start with an example:

  struct B1;
  struct B2;
  struct A1 { B1 *f(); };
  struct A2 { B2 *f(); };

This is a perfectly valid C++ snippet. In fact it could be extended to also implement `A1::f` and `A2::f` and call them and it would still not require the definitions for structs `B1` and `B2`. And I believe the ast importer will would not import their definitions even if they were available. It certainly wouldn't force them to be defined (`getExternalSource()->CompleteType(B1)`).

This changes if the methods become virtual. In this case, this code becomes valid only if `B2*` is convertible to `B1*` (i.e., `B2` is derived from `B1`). To know that, both `B1` and `B2` have to be complete. Regular clang parser will enforce that, and codegen will depend on it. However, the AST importer will not automatically import these classes. Lldb's code to do that is here https://github.com/llvm/llvm-project/blob/fdc6aea3fd822b639baaa5b666fdf7598d08c8de/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp#L994.

I don't know whether something like this would be in scope for the default ast importer, but it seemed useful to mention in the context of the present discussion.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86660/new/

https://reviews.llvm.org/D86660