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

refactor: move s2n_result functions inline #4739

Merged
merged 4 commits into from
Sep 5, 2024
Merged

Conversation

camshaft
Copy link
Contributor

@camshaft camshaft commented Aug 27, 2024

Description of changes:

This change moves all of the s2n_result functions into the s2n_result.h file. All of these functions consist of a single expression and prevent potential optimizations without LTO being enabled. All of these functions are static inline to avoid duplicate symbols.

Testing:

Since this is a simple refactor, no additional testing is needed.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions github-actions bot added the s2n-core team label Aug 27, 2024
@camshaft camshaft changed the title feat: move s2n_result functions inline refactor: move s2n_result functions inline Aug 28, 2024
@camshaft camshaft marked this pull request as ready for review August 28, 2024 16:35
Copy link
Contributor

@lrstewart lrstewart left a comment

Choose a reason for hiding this comment

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

I don't really see any downside to this change, but do you have any data on the performance benefits?

Comment on lines +18 to +20
/*
* The goal of s2n_result is to provide a strongly-typed error
* signal value, which provides the compiler with enough information
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think we generally put big comments like this after the includes, rather than before? But idk, we don't have many examples :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure i agree with that convention. Why wouldn't the module-level documentation be the first thing, since it's the first thing you read to understand why the module exists? Why would includes be first?

@camshaft
Copy link
Contributor Author

I don't really see any downside to this change, but do you have any data on the performance benefits?

Not offhand but I can try to get some if you'd like, though it may take a bit of effort. Actually, the reason I'm making this change now is to simplify the fuzz test work that @jouho is working on - we were running into issues building the LD_PRELOAD .so, which ended up missing the s2n_result_is_ok function definitions and it seemed cleaner to just make it inline, since it's such a fundamental function, rather than complicate the build even more to force it to include the s2n_result.o.

@camshaft camshaft enabled auto-merge (squash) September 4, 2024 17:25
@camshaft camshaft merged commit b1d1609 into aws:main Sep 5, 2024
36 checks passed
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.

3 participants