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

Revise dependency analysis to be more precise #525

Merged
merged 2 commits into from
Jan 31, 2025

Conversation

msprotz
Copy link
Contributor

@msprotz msprotz commented Jan 30, 2025

The dependency analysis was pretty coarse, and only represented dependencies from one "file" to either regular or internal headers. It turns out that the dependencies vary quite widely between that file's public header, internal header, or C implementation.

This was pointed out here: hacl-star/hacl-star#1007 (comment)

This revamps the analysis to now distinguish dependencies between { h file, internal/h file, c file } to { h file, internal/h file }.

The net result is a considerably reduced number of #includes in public headers.

@msprotz msprotz enabled auto-merge January 30, 2025 23:55
msprotz added a commit to hacl-star/hacl-star that referenced this pull request Jan 30, 2025
@msprotz
Copy link
Contributor Author

msprotz commented Jan 30, 2025

Error: This request has been automatically failed because it uses a deprecated version of actions/upload-artifact: v3. Learn more: https://github.blog/changelog/2024-04-16-deprecation-notice-v3-of-the-artifact-actions/

@msprotz
Copy link
Contributor Author

msprotz commented Jan 30, 2025

@mtzguido do we need the other PR merged for this to work? if so, happy to approve it (or for you to merge it)... thanks!

@mtzguido
Copy link
Member

I think this simple patch should do it. You had very valid concerns for the other CI so maybe it can wait a bit.

@msprotz msprotz merged commit 5e12cec into master Jan 31, 2025
2 checks passed
@msprotz msprotz deleted the protz_dependency_analysis branch January 31, 2025 00:38
@msprotz
Copy link
Contributor Author

msprotz commented Jan 31, 2025

Thanks!

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.

2 participants