Skip to content

Conversation

@augusto2112
Copy link

@augusto2112 augusto2112 commented Mar 28, 2020

This PR fixes the crash reported here. In addition, it improves the error message when type reconstruction fails in the SwiftASTContext class.

@augusto2112 augusto2112 changed the title Fix unkwnown type cast crash Fix unknown type cast crash Mar 28, 2020
@augusto2112 augusto2112 force-pushed the fix_crash_unkwnown_type branch from 4038311 to 4b3d055 Compare March 28, 2020 00:33
@augusto2112
Copy link
Author

Hi @vedantk, could you take a look at this, if/when you have the time? Related to this post you helped me with

@vedantk
Copy link

vedantk commented Mar 31, 2020

Hey @augusto2112, thanks for doing this, this seems like the right approach. I noticed that there are quite a few formatting diffs in the file unrelated to your change: perhaps these got pulled in by clang-format? Please update the diff to remove those unrelated changes if possible. It should be good to land afterwards.

@augusto2112
Copy link
Author

Hi @vedantk, I was thinking in doing a small change:

expose a public SwiftASTContext::ReconstructType(ConstString) (no error), and do the whole "if error, add diagnostic" here. That way we don't need AddErrorStatusAsGenericDiagnostic in SwiftASTContext's public API and we remove the "if error" logic that is duplicated in three places.

What do you think?

I noticed that there are quite a few formatting diffs in the file unrelated to your change: perhaps these got pulled in by clang-format?

Yeah, I think it was because of clang-format. I'll update that.

@vedantk
Copy link

vedantk commented Apr 1, 2020

It may be convenient in the future to have AddErrorStatusAsGenericDiagnostic as part of SwiftASTContext's public interface. I think it'd be fine to add the helper, though.

@augusto2112 augusto2112 force-pushed the fix_crash_unkwnown_type branch from 4b3d055 to 2b2672b Compare April 1, 2020 00:41
@augusto2112 augusto2112 force-pushed the fix_crash_unkwnown_type branch from 2b2672b to 0718eea Compare April 1, 2020 00:47
@augusto2112
Copy link
Author

@vedantk OK, I pushed the changes.

@vedantk
Copy link

vedantk commented Apr 1, 2020

@swift-ci test

@vedantk vedantk merged commit e7ae372 into swiftlang:swift/master Apr 1, 2020
@vedantk
Copy link

vedantk commented Apr 1, 2020

@augusto2112 could you open a PR submitting this change to apple:swift/master-next as well? Otherwise, the change will disappear after the next rebase (we do not auto merge lldb changes from swift/master to swift/master-next). If you don't have the bandwidth for that, just lmk, and I'll handle it. Thanks.

@vedantk
Copy link

vedantk commented Apr 1, 2020

(Incidentally if you've done other lldb work on swift/master, this will need to be cherry-picked to swift/master-next as well.)

@augusto2112
Copy link
Author

@vedantk Ok, will do!

(Incidentally if you've done other lldb work on swift/master, this will need to be cherry-picked to swift/master-next as well.)

I've done a couple, I usually wait to see if the tests pass before cherry-picking so I don't have to re-do the master-next in case I have to change something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants