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

Issue 471: Fix panic when removing a route that doesn't exist in w.routes. #472

Merged
merged 2 commits into from Jul 14, 2021
Merged

Issue 471: Fix panic when removing a route that doesn't exist in w.routes. #472

merged 2 commits into from Jul 14, 2021

Conversation

jeffreydwalter
Copy link
Contributor

@jeffreydwalter jeffreydwalter commented Jul 12, 2021

This PR fixes the remaining panic in RemoveRoute().

@jeffreydwalter jeffreydwalter changed the title Issue 471: Fix panic when removing a route that doesn't exist in w.ro… Issue 471: Fix panic when removing a route that doesn't exist in w.routes. Jul 12, 2021
web_service.go Outdated Show resolved Hide resolved
web_service.go Outdated Show resolved Hide resolved
@jeffreydwalter
Copy link
Contributor Author

@emicklei I made the changes you suggested. Would like to get this PR merged so I can vendor it back into my codebase. Please let me know if there's anything else I can do to facilitate. Thanks!

@emicklei emicklei merged commit f8e15b6 into emicklei:v3 Jul 14, 2021
@jeffreydwalter
Copy link
Contributor Author

@emicklei thinking about this further, what do you think about setting the capacity of newRoutes, like this: newRoutes := make([]Route, 0, len(w.routes))? This would potentially allocate an extra item in the slice in the case the route is found and removed, but would cut down on slice allocations. If you think that is better, I'm happy to make another PR.

@jeffreydwalter
Copy link
Contributor Author

@emicklei what do you say in regard to my last comment? ☝🏻

@emicklei
Copy link
Owner

@jeffreydwalter , removing a Route is not on the hot-path so any optimisations here are not significant.

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.

None yet

2 participants