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.

Sign up for GitHub

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

Jump to bottom

Remove optional crc argument to LoadDynamicModule and LoadStaticModule #4493

Merged
merged 1 commit into from
May 21, 2021
Merged

Remove optional crc argument to LoadDynamicModule and LoadStaticModule #4493

merged 1 commit into from
May 21, 2021

Conversation

fingolfin
Copy link
Member

No caller used it, and using it correctly would have been both non-trivial,
and generally of limited use (as only the arbitrary hardcoded "crc" value in
the module's StructInitInfo instance is checked)

Resolves #4235

@fingolfin fingolfin added topic: kernel release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes labels May 17, 2021
@fingolfin fingolfin force-pushed the mh/LoadDynamicModule-crc branch 3 times, most recently from 2e22e9a to 9e901c3 Compare May 18, 2021 20:20
@ChrisJefferson
Copy link
Contributor

Is the plan to (long term) get rid of crc altogether? If so, it might be worth adding/editing the comment in modules.h, to say the crc values isn't actually used any more?

@fingolfin
Copy link
Member Author

The crc is still used for files produced by gac, and there we do use it, namely to detect if the source file changed since the compilation -- i.e., GAP will detect if oper1.g is different from what was used to generate c_oper1.c, and then won't use the compiled code from c_oper1.c.

@fingolfin
Copy link
Member Author

That said, it makes sense to update the comment for the crc field to explain just that

@fingolfin
Remove crc argument to Load{Dynamic,Static}Module
6c8a15e 
No caller used it, and using it correctly would have been both non-trivial,
and generally of limited use (as only the arbitrary hardcoded "crc" value in
the module's `StructInitInfo` instance is checked)
@fingolfin
Copy link
Member Author

I've now added such a change to src/modules.h

@fingolfin
Copy link
Member Author

@ChrisJefferson OK now?

@fingolfin fingolfin merged commit ae742ec into gap-system:master May 21, 2021
@fingolfin fingolfin deleted the mh/LoadDynamicModule-crc branch May 21, 2021 17:56
@fingolfin fingolfin added the kind: removal or deprecation A feature was removed or deprecated / made obsolete label Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChrisJefferson ChrisJefferson ChrisJefferson approved these changes

@wilfwilson wilfwilson Awaiting requested review from wilfwilson

Assignees
No one assigned
Labels
kind: removal or deprecation A feature was removed or deprecated / made obsolete release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes topic: kernel
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

LoadDynamicModule: documentation for crc argument is wrong
2 participants
@fingolfin @ChrisJefferson