Page MenuHomePhabricator

Mark lambda decl as invalid if a captured variable has an invalid type.

Authored by jgorbe on Nov 14 2018, 3:26 PM.



This causes the compiler to crash when trying to compute a layout for
the lambda closure type (see included test).

Diff Detail

Event Timeline

jgorbe created this revision.Nov 14 2018, 3:26 PM



You'll need to use FieldType->getBaseElementTypeUnsafe() or similar here to handle the case where the field type is an array whose element type is a class. (That can happen in a lambda if you capture a local array variable by value.)


For consistency with the class case, I think we should mark the field invalid in this case too.

jgorbe updated this revision to Diff 174118.Nov 14 2018, 4:25 PM

Fixed some issues pointed out in review comments:

  • call to getBaseElementType before checking type validity.
  • when the type is incomplete, mark not only the lambda closure type as invalid but also the field

Also, added a couple of checks to resemble more closely the code that checks user-declared fields in classes.

jgorbe marked 2 inline comments as done.Nov 14 2018, 4:27 PM
jgorbe added inline comments.

Done, I ended up using Context.getBaseElementType because the call to RequireCompleteType below required a QualType.



Thanks! Some simplifications are possible here, but otherwise this looks good.


Hah, OK, now you've switched to using RequireCompleteType this has become redundant again (it'll walk to the base element type for you). =)


I believe the else case here is redundant: if RequireCompleteType returns false, the type is complete and there's nothing more you need to do.


Can you also add a test for the "capturing an array of incomplete type" case please?

jgorbe updated this revision to Diff 174853.Nov 20 2018, 4:13 PM
jgorbe marked 2 inline comments as done.

Added a test for the "capturing an array of incomplete type" case. See also responses to inline comments below.


Removed call to getBaseElementType(), changed appearances of EltTy to FieldType instead.


If I just remove the else block, the test case included with the patch keeps crashing. Is there any way a Decl can be invalid even after RequireCompleteType returns false?

rsmith accepted this revision.Nov 20 2018, 9:51 PM

Looks good, thanks! (Though we generally prefer fewer test files with more test per file, because a lot of the overhead of running tests is the setup / teardown time, so can you merge the two added tests into a single file?)


I think I was just wrong before :) Looks like RequireCompleteType doesn't check whether the type is invalid.

This revision is now accepted and ready to land.Nov 20 2018, 9:51 PM
jgorbe updated this revision to Diff 174944.Nov 21 2018, 9:34 AM

Folded the two test cases (capturing an invalid type and capturing an invalid array type) into a single file.

jgorbe closed this revision.Nov 21 2018, 10:05 AM