gh-121404: avoid accessing compiler internals in codegen functions #121538
Conversation
…hat caller doesn't need to access compiler internals
iritkatriel
added
skip news
interpreter-core
There was a problem hiding this comment.
I think this makes sense overall. It took a bit of reading to understand the intent, and it might be helpful to have a more descriptive commit message.
If I understand correctly, the core idea is to take four features that are tied to the current compiler_unit and hide the details of the compiler_unit in accessor functions.
I wasn't sure if there was a reason to do exactly these four, or if this was just an intermediate step and you'll cover other features in a future change.
What's the rationale for the accessor functions? I assume you'll split the code in a future change, so that the compiler_unit will not be visible in some parts of the code.
The detailed comments along the way were from me reading and understanding. For most of them, you don't need to do anything unless you want to correct a misunderstanding on my part.
| #define INSTR_SEQUENCE(C) compiler_instr_sequence(C) | ||
| #define FUTURE_FEATURES(C) compiler_future_features(C) | ||
| #define SYMTABLE(C) compiler_symtable(C) | ||
| #define SYMTABLE_ENTRY(C) compiler_symtable_entry(C) |
There was a problem hiding this comment.
How much do the macros buy us? The actual function names are simple enough and only slightly longer. It might be simpler to just drop the indirection.
There was a problem hiding this comment.
After we split the compiler to a separate file, and start thinking about making things configurable, the indirection might turn out useful (you can redefine the macro to do something else). I don't know if we will, but wanted to keep the option for now.
There was a problem hiding this comment.
I'd be inclined to add it later if it's needed (yagni and all), but I don't feel strongly about it. Whatever works for you :-).
|
Thanks. I tried to give some rationale in the issue. Eventually I want to have a small file with the compiler/ compiler_unit implementation, and a larger file that accesses them through opaque pointers (and has all the code-generation logic for individual AST nodes). The code generation file will be simple, so the complexity ends up contained in a smaller file. It needs abstractions like here, moving some functionality from the compiler to the symtable (#121272), etc. |
These are some simple transformations to hide compiler internals, as part of the work to split compile.c into codegen and compiler.
The individual commits should be easy to review.