-
-
Notifications
You must be signed in to change notification settings - Fork 7
Fixed build errors on VS2013 and earlier. #6
base: master
Are you sure you want to change the base?
Conversation
Added inline redefine. Fixed conflict with winioctl.h
| // Build cookie buffer. | ||
| uint32_t cookieSize = e->enc_.GetMagicCookieSize(e->outf_.mChannelsPerFrame); | ||
| char cookie[cookieSize]; | ||
| char* cookie = (char*)alloca(sizeof(char) * (cookieSize)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You never free this, it will leak memory...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alloca shouldn't need to be freed. Read the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're absolutely correct, nevermind :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I solved the same problems (building for a VS version < VS2013).
I would prefer a ifndef since for gcc it doesn't seem to be a problem.
See my changes here:
https://github.com/m-a-v/node-libalac/commits/master
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code should have the same effect on gcc, the alloca statement is just more specific. One would need to check if it truly gives the same code path, but I feel preprocessor directives are an "ugly" hack and should be minimized. I already hated the fact I needed to add one for __inline.
|
@EraYaN Shouldn't this be added to the |
|
I find the ARMEL patch that is in the patch folder unnecessary as well. No compiler will break if you add it into the normal code. So no? |
|
I think that it's just to document what changes have been added on top of the official Apple release, I might be wrong though... |
Added inline redefine.
Fixed conflict with winioctl.h
It does not pass the last test though on Windows (in the encoder). I only need the decoder though.