search

selmf / unarr

A decompression library for rar, tar, zip and 7z archives
GNU Lesser General Public License v3.0
70 stars 13 forks source link

Some Windows fixes #20

Closed mastercoms closed 2 years ago

mastercoms commented 2 years ago

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

selmf commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

selmf commented 2 years ago

Not sure if a new branch is really needed. 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. I will also look into adding a mingw toolchain to my CI so I can catch these issues in the future.

Am 26. Januar 2022 02:35:50 MEZ schrieb mastercoms @.***>:

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

-- Reply to this email directly or view it on GitHub: https://github.com/selmf/unarr/pull/20#issuecomment-1021772357 You are receiving this because you commented.

Message ID: @.***>

mastercoms commented 2 years ago

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

mastercoms commented 2 years ago

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 commented 2 years ago

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

selmf commented 2 years ago

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 commented 2 years ago

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