Skip to content

Conversation

@ctrlcctrlv
Copy link

Might help solve #10.

@jpsdr
Copy link
Owner

jpsdr commented Jan 30, 2023

Hello.
I will not merge the PR, some of it break my VS build. "aligned_alloc" doesn't exist, and "_aligned_free" is mandatory when an "_aligned_malloc" is made, you must use it, and not just "free".
Also, i think "strncasecmp" doesn't exist (with VS).
It also involve changes in the avisynth include files, made changes in the avisynth include files validate with avs devs. As when i update these files i just replace them with the new version i got from avs+ github.
Don't realy want to put this kind of changes.
My advice : create your own github version cloning this, and then apply your patch.

@ctrlcctrlv
Copy link
Author

No problem at all @jpsdr. I didn't open PR with any expectation of merger. It just helps users see what patches exist if they search Google, that's all. :-)

@DJATOM
Copy link

DJATOM commented Jan 31, 2023

@ctrlcctrlv You can do better and wrap compiler-specific code into preprocessor's directive, so it will be ignored for visual studio. That way it definitely will not harm the maintainer's experience with VS. You still have to provide another way (meson, cmake or make) to compile nnedi3 plugin with your additions (setting your directive with -D to enable it).

@ctrlcctrlv
Copy link
Author

ctrlcctrlv commented Feb 1, 2023

@DJATOM I have no way to test on VS+MSVC, sorry, which is why I didn't already do that.

@ctrlcctrlv
Copy link
Author

Hmm. I guess the macro is _MSC_VER.

#ifndef _MSC_VER
#define _aligned_malloc(size, alignment) aligned_alloc(size, alignment)
// Note: This is safe on Unix. see `man 3 aligned_alloc`
#define _aligned_free(m) free(m)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jpsdr You might be right about this on Windows but using free on aligned memory is fine on Unix.

@ctrlcctrlv
Copy link
Author

OK @DJATOM I did my best even w/o VisualStudio to fix this PR.

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.

3 participants