Skip to content

Remove unnecessary C-style casts with BLE UUIDs#524

Merged
JF002 merged 1 commit intoInfiniTimeOrg:developfrom
jonvmey:ble-uuid-c-casts
Oct 9, 2021
Merged

Remove unnecessary C-style casts with BLE UUIDs#524
JF002 merged 1 commit intoInfiniTimeOrg:developfrom
jonvmey:ble-uuid-c-casts

Conversation

@jonvmey
Copy link
Copy Markdown
Contributor

@jonvmey jonvmey commented Jul 24, 2021

Instead of casting the UUID object to the ble_uuid_t* used throughout
the NimBLE API just pass the address of the ble_uuid_t member that's at
the start of each of the UUID structs.

Instead of casting the UUID object to the ble_uuid_t* used throughout
the NimBLE API just pass the address of the ble_uuid_t member that's at
the start of each of the UUID structs.
@geekbozu
Copy link
Copy Markdown
Member

I kind of dislike how its just .u ...it feels ambiguous. Any reason to not change it to .uuid? Otherwise i do feel this is much clearer to read compared to before

@jonvmey
Copy link
Copy Markdown
Contributor Author

jonvmey commented Aug 21, 2021

These types are from NimBLE so you'd have to take that up with them.

@Avamander
Copy link
Copy Markdown
Collaborator

Very likely that if you'd make a PR that refactors it for niceness it will be merged, @geekbozu. The name has misled more than one person by now so why not try.

Copy link
Copy Markdown
Collaborator

@Avamander Avamander left a comment

Choose a reason for hiding this comment

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

I think it's a good change, both style and safety wise. The current casts are not great.

@JF002
Copy link
Copy Markdown
Collaborator

JF002 commented Oct 9, 2021

@Avamander

I think it's a good change, both style and safety wise. The current casts are not great.

I totally agree!

@jonvmey Thanks for this PR, and sorry for the delay to review it !

@JF002 JF002 merged commit f99f71c into InfiniTimeOrg:develop Oct 9, 2021
@JF002 JF002 added this to the 1.7.0 milestone Oct 9, 2021
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.

5 participants