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

THREAD_FREE: rely less on macros #11943

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

moonchen
Copy link
Contributor

@moonchen moonchen commented Jan 7, 2025

Move as much of the implementation of THREAD_FREE as possible into a type-safe function. This was already done for THREAD_ALLOC.

Move as much of the implementation of THREAD_FREE as possible into a
type-safe function.  This was already done for THREAD_ALLOC.
@moonchen moonchen added this to the 10.1.0 milestone Jan 7, 2025
Copy link
Contributor

@bneradt bneradt left a comment

Choose a reason for hiding this comment

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

Good change.

Do you think thread_free should be marked inline?

@moonchen
Copy link
Contributor Author

moonchen commented Jan 7, 2025

Good change.

Do you think thread_free should be marked inline?

I don't know. We don't need it to comply with ODR since the function is a template function. My understanding is that modern compilers are already smart about whether to inline the function, so it's hard to say whether adding the keyword will help with performance.

@bneradt
Copy link
Contributor

bneradt commented Jan 8, 2025

In case it's helpful, here's the clang analyzer report from the artifact's directory of the job:
https://ci.trafficserver.apache.org/job/Github_Builds/job/clang-analyzer/5363/artifact/output/11943/scan-build-2025-01-07-23-04-27-974869-ju80g4aa/index.html

@moonchen
Copy link
Contributor Author

[approve ci autest]

@moonchen moonchen marked this pull request as draft January 13, 2025 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants