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

Various fixes resulting form Python testing #63

Merged
merged 9 commits into from
Aug 6, 2011
Merged

Various fixes resulting form Python testing #63

merged 9 commits into from
Aug 6, 2011

Conversation

max99x
Copy link
Contributor

@max99x max99x commented Aug 5, 2011

This pull includes a bunch of small fixes for errors I've discovered while testing empythoned. There's no unifying theme, so just check the commit messages. All llvm_gcc_0_0 tests pass.

@kripken
Copy link
Member

kripken commented Aug 5, 2011

  1. Please use |Date.now()| instead of |new Date().getTime()|.
  2. A shared library should never do static allocation, since it uses the main file's allocation stuff, I think? Let's make sure that is fixed in general instead of modifying specific appearances of ALLOC_STATIC to ALLOC_NORMAL. How about in the shared library preamble, defining a var ALLOC_STATIC to have the value of ALLOC_NORMAL?

@max99x
Copy link
Contributor Author

max99x commented Aug 6, 2011

  1. Date.now() is not supported on some browsers, e.g. IE8. I don't suppose Emscripten compatible with IE8, is it?
  2. I'm a little wary of shadowing a constant, and that is currently the only place where static allocations happen in shared libraries, since they have their own stripped down preamble/postamble and don't include library*.js.

Added a note about shared lib static allocations.
@kripken
Copy link
Member

kripken commented Aug 6, 2011

  1. Well, IE8 also doesn't support things like Canvas, and worse has such a slow JS engine it's doubtful Emscripten would be very useful for it. IE9 is really the first modern version. I would rather not clutter our code to support IE8, so I prefer Date.now().
  2. Good point about the allocation stuff. Ok, let's do it that way, but please add a comment near that change regarding it being the only place we would potentially do static allocation in a shared library.

@max99x
Copy link
Contributor Author

max99x commented Aug 6, 2011

Done.

kripken added a commit that referenced this pull request Aug 6, 2011
Various fixes resulting form Python testing
@kripken kripken merged commit cca5278 into emscripten-core:master Aug 6, 2011
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.

None yet

2 participants