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.

Sign up for GitHub

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

Jump to bottom

Unknown constant collides #1293

Closed
wdouglass opened this issue Jul 1, 2020 · 6 comments · Fixed by #1929
Closed

Unknown constant collides #1293

wdouglass opened this issue Jul 1, 2020 · 6 comments · Fixed by #1929
Assignees
@wdouglass

Comments

@wdouglass
Copy link
Member

The Unknown constant in constants.go is set with iota, which in it's block always resolves to 0. This collides in a lot of places; This issue is a discussion point.

should we set "Unknown" to an arbitrary value? should we start all of our enumerations at iota+1? not sure how to resolve this, but it may be the source of a few bugs!
@wdouglass wdouglass self-assigned this Jul 1, 2020
@wdouglass
Copy link
Member Author

I'm happy to refactor to resolve this, but let's discuss a solution first.

@Sean-Der
Copy link
Member

Sean-Der commented Aug 6, 2021 •
edited
Loading

Hey @wdouglass

Sorry I never responded to this. I am just starting to catch up on things for v3.1.0. I think we should remove Unknown entirely. This does seem like a major source of bugs!

Instead for every enum we should have a dedicated Unknown like RTPTransceiverDirectionUnknown

I am also in support of requiring iota + 1, but that seems like people could make mistakes.

@Sean-Der Sean-Der closed this as completed Aug 6, 2021
@Sean-Der Sean-Der reopened this Aug 6, 2021
@wdouglass
Copy link
Member Author

Hey @wdouglass

Sorry I never responded to this. I am just starting to catch up on things for v3.1.0. I think we should remove Unknown entirely. This does seem like a major source of bugs!

Instead for every enum we should have a dedicated Unknown like RTPTransceiverDirectionUnknown

I am also in support of requiring iota + 1, but that seems like people could make mistakes.

I think that's a great solution! thanks!

@Sean-Der
Copy link
Member

Sean-Der commented Aug 6, 2021

@wdouglass you are fast :)

You interested in taking that on? Happy to merge a PR right away, would love to have you back!

@Sean-Der Sean-Der added this to the 3.1.0 milestone Aug 6, 2021
@wdouglass
Copy link
Member Author

yeah, i could try to put a patch together later today

@wdouglass wdouglass mentioned this issue Aug 6, 2021
Merged
@Sean-Der Sean-Der modified the milestones: 3.1.0, 4.0.0 Aug 22, 2021
@Sean-Der
Copy link
Member

Moving to v4.0.0 this would be an API break unfortunately

@wdouglass wdouglass mentioned this issue Aug 24, 2021
Merged
wdouglass added a commit that referenced this issue Aug 24, 2021
@wdouglass
Remove the "Unknown" constant
1d30c2b 
This commit replaces the Unknown constant with
separate constants for each enumeration that
uses it.

Fixes #1293
pionbot pushed a commit that referenced this issue Aug 24, 2021
@wdouglass
Remove the "Unknown" constant
08be4d5 
This commit replaces the Unknown constant with
separate constants for each enumeration that
uses it.

Fixes #1293
wdouglass added a commit that referenced this issue Aug 24, 2021
@wdouglass
Remove the "Unknown" constant
66987c3 
This commit replaces the Unknown constant with
separate constants for each enumeration that
uses it.

Fixes #1293
pionbot pushed a commit that referenced this issue Aug 24, 2021
@wdouglass
Remove the "Unknown" constant
1074371 
This commit replaces the Unknown constant with
separate constants for each enumeration that
uses it.

Fixes #1293
wdouglass added a commit that referenced this issue Aug 24, 2021
@wdouglass
Remove the "Unknown" constant
cfd7cac 
This commit replaces the Unknown constant with
separate constants for each enumeration that
uses it.

Fixes #1293
wdouglass added a commit that referenced this issue Aug 24, 2021
@wdouglass
Remove the "Unknown" constant
4af2175 
This commit replaces the Unknown constant with
separate constants for each enumeration that
uses it.

Fixes #1293
wdouglass added a commit that referenced this issue Aug 24, 2021
@wdouglass
Remove the "Unknown" constant
d3c0898 
This commit replaces the Unknown constant with
separate constants for each enumeration that
uses it.

Fixes #1293
wdouglass added a commit that referenced this issue Aug 25, 2021
@wdouglass
Remove the "Unknown" constant
8f572e7 
This commit replaces the Unknown constant with
separate constants for each enumeration that
uses it.

Fixes #1293
@Sean-Der Sean-Der removed this from the 4.0.0 milestone May 22, 2022
@Sean-Der Sean-Der mentioned this issue May 15, 2023
Merged
Sean-Der pushed a commit that referenced this issue Sep 12, 2023
@wdouglass @Sean-Der
Remove the "Unknown" constant
75db665 
This commit replaces the Unknown constant with
separate constants for each enumeration that
uses it.

Fixes #1293
Sean-Der pushed a commit that referenced this issue Sep 12, 2023
@wdouglass @Sean-Der
Remove the "Unknown" constant
127387e 
This commit replaces the Unknown constant with
separate constants for each enumeration that
uses it.

Fixes #1293
Sean-Der pushed a commit that referenced this issue Sep 12, 2023
@wdouglass @Sean-Der
Remove the "Unknown" constant
2c5d35d 
This commit replaces the Unknown constant with
separate constants for each enumeration that
uses it.

Fixes #1293
Sean-Der pushed a commit that referenced this issue Sep 12, 2023
@wdouglass @Sean-Der
Remove the "Unknown" constant
1345033 
This commit replaces the Unknown constant with
separate constants for each enumeration that
uses it.

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

@wdouglass wdouglass

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove the "Unknown" constants pion/webrtc
2 participants
@Sean-Der @wdouglass