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

Move C fallback functions into arch/generic [Part 1] #1630

Merged
merged 3 commits into from
Jan 19, 2024
Merged

Conversation

Dead2
Copy link
Member

@Dead2 Dead2 commented Jan 3, 2024

Due to the scope of this cleanup, this also includes a major cleanup of how we use includefiles, and also ended up cleaning up unneeded includefiles and also some incorrect dependency rules in win32 makefiles.

This PR is limited in scope to doing the following:

  • Simplify how we use includes
  • Move adler32 fallbacks
  • Move crc32 fallbacks

Down the line, this will make it more easily possible to make support for compiling zlib-ng without including the C fallback functions. The grouping of these objects can also (depending on linker behavior) slightly improve instruction cache efficiency for the rest of the code, since they will no longer be interspersed between more important (for performance) code.

@Dead2 Dead2 added the cleanup Improving maintainability or removing code. label Jan 3, 2024
Copy link

codecov bot commented Jan 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (70e2fc7) 83.12% compared to head (96a0978) 82.97%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1630      +/-   ##
===========================================
- Coverage    83.12%   82.97%   -0.15%     
===========================================
  Files          133      135       +2     
  Lines        10897    10885      -12     
  Branches      2817     2732      -85     
===========================================
- Hits          9058     9032      -26     
- Misses        1130     1143      +13     
- Partials       709      710       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nmoinvaz
Copy link
Member

nmoinvaz commented Jan 3, 2024

Looks good. Not sure if crc32_braid_c files needs to be suffixed with _c.

@Dead2
Copy link
Member Author

Dead2 commented Jan 3, 2024

Looks good. Not sure if crc32_braid_c files needs to be suffixed with _c.

We still have crc32_braid.c in the root folder, so it avoids a conflict. Also I figured perhaps we just slap on the _c for all the files that are only for the C-fallback functions. This makes it easier to spot what files those are, and easier to avoid the mistake of adding non-C-fallback code to those files.

@Dead2
Copy link
Member Author

Dead2 commented Jan 3, 2024

@nmoinvaz Btw, the linter doesn't like TABs in Makefiles, but the Makefiles break without TABs. Perhaps separate rules can be made for those files?

@nmoinvaz
Copy link
Member

nmoinvaz commented Jan 3, 2024

Perhaps change lint.yaml to:

git config core.whitespace blank-at-eol

@nmoinvaz nmoinvaz added the Rebase needed Please do a 'git rebase develop yourbranch' label Jan 5, 2024
@Dead2 Dead2 force-pushed the no-c-fallback branch 2 times, most recently from 06949b1 to a142c2f Compare January 15, 2024 12:07
@Dead2 Dead2 removed the Rebase needed Please do a 'git rebase develop yourbranch' label Jan 15, 2024
@Dead2 Dead2 merged commit 06895bc into develop Jan 19, 2024
270 of 271 checks passed
@Dead2 Dead2 deleted the no-c-fallback branch February 22, 2024 18:57
@Dead2 Dead2 mentioned this pull request May 30, 2024
@Dead2 Dead2 mentioned this pull request Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Improving maintainability or removing code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants