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

cdef: enforce 8 element size for first_max_element #2807

Closed
wants to merge 1 commit into from

Conversation

tmatth
Copy link
Member

@tmatth tmatth commented Sep 15, 2021

This is not needed currently as cost is always 8 elements long but will guard against
unexpected breakage should the function be used on single element lists in the future.

@tmatth tmatth force-pushed the single-element-max-test branch from 1f796d6 to c7c9a5c Compare September 15, 2021 20:26
@coveralls
Copy link
Collaborator

coveralls commented Sep 15, 2021

Coverage Status

Coverage decreased (-0.07%) to 83.315% when pulling 2939576 on tmatth:single-element-max-test into 2ec4e67 on xiph:master.

@redzic
Copy link
Collaborator

redzic commented Sep 15, 2021

I think it would be better to just enforce that the function can only be called on 8-element lists by just taking a &[i32; 8] as the argument, as this also allows for better optimization

@tmatth tmatth force-pushed the single-element-max-test branch from c7c9a5c to 2939576 Compare September 15, 2021 20:57
@tmatth tmatth changed the title test: add first_max_element check for 1-elem lists cdef: enforce 8 element size for first_max_element Sep 15, 2021
@redzic
Copy link
Collaborator

redzic commented Sep 15, 2021

Btw my PR (#2806) also already does this, and also updates the doc comment

@tmatth
Copy link
Member Author

tmatth commented Sep 15, 2021

Btw my PR (#2806) also already does this, and also updates the doc comment

Ah ok will close.

@tmatth tmatth closed this Sep 15, 2021
@tmatth tmatth deleted the single-element-max-test branch September 15, 2021 21:05
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