Update compiler.rst for more precision about new opcode #416
Conversation
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. You can check yourself to see if the CLA has been received. Thanks again for your contribution, we look forward to reviewing it! |
|
I have just signed the CLA. |
There was a problem hiding this comment.
Thanks @psyker156 for the PR. A small wording suggestion for clarity.
compiler.rst
Outdated
| will take effect only after running ``make regen-importlib``. | ||
| will take effect only after running ``make regen-importlib``. Running this | ||
| command before adding the new bytecode to :file:`Python/ceval.c` will result | ||
| in an error. You should proceed to the next step before importlib is |
There was a problem hiding this comment.
You should ... regenerated
Instead, you should add the new bytecode and then run make regen-importlib.
There was a problem hiding this comment.
Fair point. What would you think of:
"Running this command before adding the new bytecode target to :file:Python/ceval.c will result in an error. You should only run make regen-importlib after the new bytecode target has been added."
Or maybe we can remove the second sentence. I was referring to the next step as that one is the one explaining how to deal with the ceval.c file. Thanks for your input!
There was a problem hiding this comment.
Your suggested wording works for me @psyker156 Thanks 👍
There was a problem hiding this comment.
Alright! I will patch it up! Thanks a lot!
|
Thanks @psyker156 for the contribution. |
|
@willingc My pleasure! Nice community. I look forward to a more active contribution pattern! |
| will take effect only after running ``make regen-importlib``. | ||
| will take effect only after running ``make regen-importlib``. Running this | ||
| command before adding the new bytecode target to :file:`Python/ceval.c` will | ||
| result in an error. You should only run ``make regen-importlib`` after the new |
There was a problem hiding this comment.
Aren't the last two sentences just repeat the same?
Closes python#417. * Update compiler.rst for more precision about new opcode * Update compiler.rst
When adding a new opcode, if the guide is followed as is, the following error will be emitted as regen-importlib expects the new opcode target to be present in ceval.c before the command is added. I simply added a quick precision in the guide. Will create an issue with this in a few seconds.
gcc -pthread -c -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -std=c99 -Wextra -Wno-unused-result -Wno-unused-parameter -Wno-missing-field-initializers -Werror=implicit-function-declaration -I. -I./Include -DPy_BUILD_CORE -o Python/ceval.o Python/ceval.c In file included from Python/ceval.c:629:0: Python/ceval.c: In function ‘_PyEval_EvalFrameDefault’: Python/opcode_targets.h:119:5: error: label ‘TARGET_LOAD_BUILTIN’ used but not defined &&TARGET_LOAD_BUILTIN, ^ Makefile:1613: recipe for target 'Python/ceval.o' failed make: *** [Python/ceval.o] Error 1