Skip to content

[lldb/formatter] Add Swift.UnsafeBufferPointer data formatter #1276

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 3, 2020

Conversation

medismailben
Copy link

@medismailben medismailben commented May 28, 2020

This patch adds support for Swift's UnsafeBufferPointer and
UnsafeMutableBufferPointer data formatting.

It introduces a summary provider and a synthetic frontend creator for
these types.

Signed-off-by: Med Ismail Bennani [email protected]

@medismailben medismailben requested review from dcci and fredriss May 28, 2020 14:30
@medismailben
Copy link
Author

@swift-ci test

@medismailben medismailben changed the title [lldb/formatter] Add Swift.UnsafeBufferPointer data formatter [WIP][lldb/formatter] Add Swift.UnsafeBufferPointer data formatter May 28, 2020
Copy link
Member

@dcci dcci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking care of this. While I think the logic is mostly correct, this opens the opportunity to do some things better. I outlined some of my concerns inline.

@medismailben medismailben force-pushed the swift/master branch 2 times, most recently from 26566a6 to ef18610 Compare May 30, 2020 03:07
@medismailben
Copy link
Author

medismailben commented May 30, 2020

I updated the patch to address @dcci and @fredriss comments:

  • I reimplemented the logic to read the entire contiguous buffer once (instead of doing a read per entry). I also added support for complex generic types like struct and enum and added comments and documentation to explain the 'value-providing synthetic children' workaround and clarify the UnsafeBufferPointer structure layout.
  • As suggested, I unified the mutable and immutable data-formatter registration.
  • Finally, I rewrote the test to prevent the UB, did some cleanup and added more test cases.

@medismailben medismailben force-pushed the swift/master branch 2 times, most recently from 4653409 to ad27f43 Compare May 30, 2020 05:40
@medismailben
Copy link
Author

@swift-ci test

@medismailben
Copy link
Author

@swift-ci test

@medismailben
Copy link
Author

@swift-ci test

@medismailben
Copy link
Author

medismailben commented Jun 2, 2020

Added more tests (value associated enums, generic class, existential type (archetypes) ...)
Addressed recent feedbacks.
Removed copyright header in tests.

@medismailben medismailben changed the title [WIP][lldb/formatter] Add Swift.UnsafeBufferPointer data formatter [lldb/formatter] Add Swift.UnsafeBufferPointer data formatter Jun 2, 2020
@medismailben
Copy link
Author

The macOS failure happened on a Swift test. Given that I only touched to LLDB, I assume this is a flaky test.

@medismailben
Copy link
Author

@swift-ci test macOS Platform

@medismailben
Copy link
Author

medismailben commented Jun 3, 2020

swiftlang/swift#32152 Should fix the test failure. Let's try re-running the macOS CI again!

@medismailben
Copy link
Author

@swift-ci test macOS Platform

@medismailben medismailben requested a review from fredriss June 3, 2020 11:52
Copy link

@fredriss fredriss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After you address this last round of comments, this LGTM

This patch adds support for Swift's UnsafeBufferPointer and
UnsafeMutableBufferPointer data formatting.

It introduces a summary provider and a synthetic frontend creator for
these types.

Signed-off-by: Med Ismail Bennani <[email protected]>
@medismailben
Copy link
Author

@swift-ci test

@medismailben
Copy link
Author

@swift-ci test macOS Platform

@medismailben medismailben merged commit 14b54c5 into swiftlang:swift/master Jun 3, 2020
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.

3 participants