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

Build fix for Visual Studio #3

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Build fix for Visual Studio #3

wants to merge 4 commits into from

Conversation

syoyo
Copy link

@syoyo syoyo commented Aug 3, 2015

minilisp is great!

And I made a patch making it work on Windows + Visual Studio 2013.

@andrewchambers
Copy link

It might be better to just have a windows branch to keep the main code tidy.

@rui314
Copy link
Owner

rui314 commented Aug 6, 2015

I agree with @andrewchambers. This patch is not actually bad, but the spirit of this interpreter is (extreme) minimalism, so we don't want to have #ifdefs in the code.

@mattn mattn mentioned this pull request Jul 5, 2016
@michaeljclark
Copy link

if you want to keep the code simple then you might just use malloc/free unconditionally and not include <sys/mman.h> because you would then be standard C compatible and that is the simplest. but I understand why you might prefer to use mmap/munmap because the system allocator may not remove the footprint of freed memory. you have more precise control using mmap/munmap. in any case I made a slightly different patch in my windows branch to use VirtualAlloc/VirtualFree. it adds a dealloc_semispace to match the alloc_semispace but i'm guessing you won't like it as it also has some #ifdef.

michaeljclark@d1cb514

if you don't want to use #ifdef then wouldn't you want to eliminate the preprocessor altogether? it's a pity you can't rely on adjacent stack variables being physically adjacent because then you could just wire root to point to the first frame variable and you wouldn't need the DEFINE macros.

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.

4 participants