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

Some Windows fixes #20

Merged
merged 4 commits into from
Feb 2, 2022
Merged

Some Windows fixes #20

merged 4 commits into from
Feb 2, 2022

Conversation

mastercoms
Copy link
Contributor

Hi, pushed some fixes for Windows, mostly for mingw.

@selmf
Copy link
Owner

selmf commented Jan 23, 2022

Thanks for the PR. I have no issue with adding the first two commits, but the third commit which attempts to fix the redefinition of __forceinline is a bit problematic.

I have not written the code segments with __forceinline myself, so I don't know about the profiling and performance tests involved that led to the decision to force inlining, but it is clear that the original code's intention is to define __forceinline to inline in contexts where __forceinline is not available.

The fix you're proposing thus has two issues:

  1. It is too complicated and there is no explanation why it is needed
  2. It tries to force __forceinline to inline in a context where it is not needed

To fix this, the best approach would probably to just use __forceinline as MinGW defines it. That should not take more than one or two lines of code :)

If you want to go the extra mile, you can also define __forceinline as "inline attribute((always_inline))" for GCC and compatible compilers, so we get the same behavior. Just don't forget to leave regular inline as fallback.

Last but not least, redefining __forceinline this way is probably an antipattern and I should consider to do some refactoring to avoid issues like this in the future.

@mastercoms
Copy link
Contributor Author

mastercoms commented Jan 23, 2022

Totally agree there, I just wasn't entirely sure what the intent was so tried to get the same behavior without redefining, which caused a warning. I'll look into skipping it.

@mastercoms
Copy link
Contributor Author

I'll remove the last commit and work on it in a new branch.

@selmf
Copy link
Owner

selmf commented Jan 26, 2022 via email

@mastercoms
Copy link
Contributor Author

mastercoms commented Jan 26, 2022

By the way, are you interested in GitHub Actions CI? I could do that in another PR if you are.

@mastercoms
Copy link
Contributor Author

mastercoms commented Jan 26, 2022

It should be enough to change the msvc check to a win32 check or add a mingw check since this behavior seems to be compiler specific.

The problem is that __forceinline has different sematics in mingw, due to being GCC based. It does not compile without overriding __forceinline for the file.

@mastercoms
Copy link
Contributor Author

Refactored the force inline usage to use a custom define, and used the suggested attribute on non-MSVC.

@selmf
Copy link
Owner

selmf commented Feb 2, 2022

Looks good :)

One more minor request: Could you change the macro name to UNARR_FORCE_INLINE? MY_FORCE_INLINE is already used by the third party code from 7z/LZMA sdk and I would like to keep the namespaces apart if possible.

Please also let me know how I should add you to the list of authors.

@mastercoms
Copy link
Contributor Author

Done, and I'd prefer to use my username. My email is mastercoms@tuta.io.

@selmf selmf merged commit 1e9892b into selmf:master Feb 2, 2022
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