Refactor emscripten py - method extraction#5137
Refactor emscripten py - method extraction#5137jgravelle-google merged 52 commits intoemscripten-core:incomingfrom
Conversation
…h is another reference to the same dict
…nction_table_data variable
|
Overall this looks nicer and cleaner to me. Only style thing that feels odd is the double newlines, is that a standard convention? |
|
Yep, in PEP8: https://www.python.org/dev/peps/pep-0008/#blank-lines I guess what I'm getting at is I don't much care either way really, but figured if I'm prettifying things I might as well do the python-standard thing by default. |
|
Cool, yeah, sounds good to do it that way. |
| 'coerced_access': coercer(access), | ||
| } | ||
| getter = ''' | ||
| function get{name}(ptr) {{ |
There was a problem hiding this comment.
So this is interesting.
I'm using '{a} {b}'.format(a=1, b=2) instead of '%s %s' % (1, 2) here so I can give the parameters more explanatory names.
But then because we're writing a javascript function, the string contains {s, which we then have to escape as {{s, which was pretty non-obvious.
On top of that I pass the format dictionary with **kwargs, which I feel is one of the less obvious bits of python syntax.
I personally like the way this looks, but is this sufficiently non-obvious that %ss would be better?
There was a problem hiding this comment.
It can't, that would be much more immediately readable as an escape sequence
The problem is the .format method interpreting the brace as part of its control, whereas \{ would help if it were a language-level escaping issue, this is a library-level interpretation issue, so no dice.
https://docs.python.org/2/library/string.html#format-string-syntax
I'm inclined to leave it, because I suspect it's a lot less weird looking if the reader is familiar with the format function. It's also still valid JS with the extra braces, so if it ever gets naively changed to %s-style in the future it'll still work. And if someone thinks "we don't need these I can remove a brace" they'll hit the KeyError I ran in to (because format then interprets the entire function body as the key). So it's ultimately pretty harmless.
| ('F32', float_coerce, 2), # TODO: fround when present | ||
| ('F64', float_coerce, 3), | ||
| ] | ||
| first_in_asm += ''.join([make_get_set(*args) for args in get_set_types]) + '\n' |
There was a problem hiding this comment.
Ditto here, I feel the *args is maybe a bit too cute. But I don't want to repeat the variable names because I feel that actually obscures the understanding here, which is "call the function with these sets of arguments"
Which of the following is best?
make_get_set(*args) for args in get_set_typesmake_get_set(a, b, c) for a, b, c in get_set_typesmake_get_set(name, coercer, shift) for name, coercer, shift in get_set_types
There was a problem hiding this comment.
Cool, I agree
Thanks
|
We should probably merge this before it gets too big or long-lived. Running the |
|
|
|
I'd say this needs all the tests passing before merge - I'd run the main test suite, |
|
Sounds right. I'll try the |
|
|
2 participants
Still working on this, but wanted to post what I've got so far.
Goal: Make emscripten.py easier to understand.
At the moment I've been mainly decomposing the four big sub-functions that make up
emscriptinto smaller functions with more explicit input/output. Haven't touched yet but am planning to:finalize_outputandemscript_wasm_backend.function_tables_and_exportsstill needs work too.Looking for feedback on:
in roughly that order.
Been testing by running the core tests.