Skip to content
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

Homogenize const types with usage in action and type storage #87

Merged
merged 1 commit into from
May 19, 2020

Conversation

tfoote
Copy link
Contributor

@tfoote tfoote commented Apr 2, 2020

This is a follow up to the Aggregate Foxy Message API Review

In both Marker and ImageMarker the constants are all uint8

uint8 CIRCLE=0
uint8 LINE_STRIP=1
uint8 LINE_LIST=2
uint8 POLYGON=3
uint8 POINTS=4
uint8 ADD=0
uint8 REMOVE=1

However the action and type they are used in are int32

# One of the above types, e.g. CIRCLE, LINE_STRIP, etc.
int32 type
# Either ADD or REMOVE.
int32 action

This proposes to homogenize them by changing the const's declared type to be consistent.

@tfoote tfoote self-assigned this Apr 2, 2020
@dirk-thomas
Copy link
Member

Was the alternative considered - to change the type of the type and action fields. 8 bit seems more than enough to encode the possible values. If both options were considered I think it would be good to mention the rationale here why one was chosen over the other.

@tfoote
Copy link
Contributor Author

tfoote commented Apr 8, 2020

That would be possible but would be changing the message format. Which would then require bag migrations and other mechanisms. Right now the consts are just being implicitly cast to the larger type when assigned into the message and consequently this is the least disruptive resolution.

@tfoote tfoote force-pushed the tfoote/homogenize_consts branch from 1f24554 to c241b99 Compare May 18, 2020 23:31
@tfoote
Copy link
Contributor Author

tfoote commented May 18, 2020

Rebased with basic CI to test code changes

Linux Build Status

@tfoote tfoote merged commit 01bb4c4 into master May 19, 2020
@tfoote tfoote deleted the tfoote/homogenize_consts branch May 19, 2020 08:38
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