Skip to content

[lldb/formatter] Add Swift.UnsafeRawBufferPointer data formatter #1314

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 13, 2020

Conversation

medismailben
Copy link

@medismailben medismailben commented Jun 5, 2020

This patch adds support for Swift's UnsafeRawBufferPointer and
UnsafeMutableRawBufferPointer data formatting.

This commit also introduces a new SwiftUnsafeType factory to adapt and
use both Unsafe{,Raw}BufferPointer with the pre-existing data formatter.

rdar://63946760

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

@medismailben
Copy link
Author

@swift-ci test

@medismailben
Copy link
Author

@swift-ci test Linux Platform

Copy link

@jimingham jimingham left a comment

Choose a reason for hiding this comment

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

This looks pretty good. The synthetic provider is leaking its SwiftUnsafeBufferPointer object, and I think you can isolate the choice to make the raw vrs. not raw difference. Once you've made the right object only call base class methods so you should always handle the base class and let a factory make the right one for you.

@medismailben
Copy link
Author

medismailben commented Jun 5, 2020

Thanks for the feedbacks @jimingham ! Indeed the factory approach would be a good way to manage both types. I'll wait to see if @dcci and @fredriss could think of other improvements I could make then update the PR.

@medismailben medismailben changed the title [lldb/formatter] Add Swift.UnsafeRawBufferPointer data formatter [WIP][lldb/formatter] Add Swift.UnsafeRawBufferPointer data formatter Jun 5, 2020
@hixio-mh
Copy link

hixio-mh commented Jun 7, 2020

Merge pull requested

@medismailben
Copy link
Author

I updated the patch addressing @jimingham comments:

  • Added a SwiftUnsafeType factory for UnsafeBufferPointer and UnsafeRawBufferPointer
  • Used std::unique_ptr instead of a raw pointer.
  • Added the test Jim requested.

@medismailben
Copy link
Author

@swift-ci test

Copy link

@jimingham jimingham left a comment

Choose a reason for hiding this comment

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

That looks better to me.

You have to handle the case where the valobj is a typedef to UnsafeBufferPointer or UnsafeRawBufferPointer. Just strip off typedef's before looking for the "Raw" in the typename.

You might also want to think a bit about "if this were to fail in the field, where would I wish I had put in logging to help me figure out what went wrong" and if there are any obvious places add some "formatter" channel logging

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.

A round of comments.

@medismailben
Copy link
Author

@swift-ci test

@medismailben medismailben changed the title [WIP][lldb/formatter] Add Swift.UnsafeRawBufferPointer data formatter [lldb/formatter] Add Swift.UnsafeRawBufferPointer data formatter Jun 12, 2020
@medismailben medismailben requested review from dcci and jimingham June 12, 2020 21:17
Copy link

@jimingham jimingham left a comment

Choose a reason for hiding this comment

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

A few nits and one annoyance w.r.t. typedef's.

@medismailben
Copy link
Author

Addressed final round of comments:

  • Handled typedef chaining and added test for it
  • Fixed typos.
  • Added logging.

This patch adds support for Swift's UnsafeRawBufferPointer and
UnsafeMutableRawBufferPointer data formatting.

This commit also introduces a new SwiftUnsafeType factory to adapt and
use both Unsafe{,Raw}BufferPointer with the pre-existing data formatter.

rdar://63946760

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

@swift-ci test

@medismailben medismailben merged commit 781480b into swiftlang:swift/master Jun 13, 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.

4 participants