Skip to content

Move entry_table to separated header file. #493

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

Merged
merged 2 commits into from
Apr 15, 2014

Conversation

aitjcize
Copy link
Contributor

Like what CPython did.
Also change the name MICROPY_USE_COMPUTED_GOTO to MICROPY_USE_COMPUTED_GOTOS (like Cpython)

@dpgeorge
Copy link
Member

Thanks, but could you please rename the new file to 'vmtable.h' (or 'vmentrytable.h')?

@aitjcize
Copy link
Contributor Author

Done.

dpgeorge added a commit that referenced this pull request Apr 15, 2014
Move entry_table to separated header file.
@dpgeorge dpgeorge merged commit 3b108e7 into micropython:master Apr 15, 2014
@aitjcize aitjcize deleted the patch branch April 15, 2014 13:33
@pfalcon
Copy link
Contributor

pfalcon commented Apr 15, 2014

Well, we should not try to mimic CPython. Vice versa, we should strive for original, as clean-room as possible implementation. No specific comments on file splitting (I usually like that), but "GOTOS" imho sounds and looks worse than "GOTO".

@aitjcize
Copy link
Contributor Author

Well, I didn't look at CPython's code on my initial patch, but still find that separated that entry table is a good thing.
I've no problem with GOTOS or GOTO, just pick that up when I look at CPython's implementation.

@lurch
Copy link
Contributor

lurch commented Apr 15, 2014

Whenever I see a chunk of code that "looks" auto-generated with lots of repeated values, like

     [MP_BC_LOAD_CONST_INT] = &&entry_MP_BC_LOAD_CONST_INT,
     [MP_BC_LOAD_CONST_DEC] = &&entry_MP_BC_LOAD_CONST_DEC,
     [MP_BC_LOAD_CONST_ID] = &&entry_MP_BC_LOAD_CONST_ID,
     [MP_BC_LOAD_CONST_BYTES] = &&entry_MP_BC_LOAD_CONST_BYTES,

my initial instinct is to write a small script to actually auto-generate it. But in this case I'll resist scratching this particular itch ;) (see where I got carried away in #481 with overcomplicating things) - I guess vmentrytable.h is probably not going to be changing much, if at all?

Would it make sense for vmentrytable.h to #include "bc0.h" to guard against things getting included in the wrong order?

@aitjcize
Copy link
Contributor Author

A script for generating that would be cool, I'll create a PR for that later.
and vmentrytable.h should not include bc0.h, since it is included in the middle of a function.

@pfalcon
Copy link
Contributor

pfalcon commented Apr 15, 2014

Would it make sense for vmentrytable.h to #include "bc0.h" to guard against things getting included in the wrong order?

You read https://github.com/micropython/micropython/blob/master/CODECONVENTIONS.md ? ;-). See commit history of that for comments discussing that requirement.

@pfalcon
Copy link
Contributor

pfalcon commented Apr 15, 2014

A script for generating that would be cool, I'll create a PR for that later.

-1. It's not hard to keep that file up to date manually, especially now that @dpgeorge announced that all bytecode implemented. We already autogenerate some stuff, may have real need to autogenerate more. Don't make us being lost in autogenerations and meta-levels. KISS

@lurch
Copy link
Contributor

lurch commented Apr 15, 2014

Ah, thanks for helping me to fill in the gaps in my understanding :)

@dpgeorge
Copy link
Member

I would say no autogeneration on this. I want the py/ core to stay as simple as possible in terms of build process, so that it can be easily included in other projects (and so I/we don't have heaps of requests for help from others who are trying to embed uPy!).

Byte codes are not going to change. And the order of the byte codes as they are written in vmentrytable.h don't have to match the numerical order of the byte codes. That's the magic of [..] array initialisation.

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