Fix a number of issues with compiler.rst #192

DimitrisJim · 2017-05-07T18:38:34Z

Closes #181.

The changes are:

Use :file: directives where necessary.
Use backticks for Function names/macros/types.
Add a code-block: c for the C snippet.
Add arguments to all macro/function signatures.
Remove obsolete macros NEW_BLOCK and rename asdl_seq_new to _Py_asdl_seq_new.

Some reformatting of the paragraphs also took place as needed when the previous changes where made.

terryjreedy · 2017-05-07T19:51:13Z

compiler.rst

-1. Parse the source code into a parse tree (Parser/pgen.c)
-2. Emit bytecode based on the parse tree (Python/compile.c)
+1. Parse the source code into a parse tree (:file:`Parser/pgen.c`)
+2. Emit bytecode based on the parse tree (:file:`Python/compile.c`)


Delete history. It is irrelevant to current development.

This comment is meant to be part of review below. I clicked wrong button when submitting it.

Hm, just to make sure I do the correct amendment, are you suggesting I drop all historical references? In short, the section can start out with:

In CPython, the compilation from source code to bytecode involves three steps:

Parse source code into a parse tree (Parser/pgen.c)

Transform parse tree into an Abstract Syntax Tree (Python/ast.c)

Transform AST into a Control Flow Graph (Python/compile.c)

Emit bytecode based on the Control Flow Graph (Python/compile.c)

The purpose of this document is to outline how these three steps of the process work.

and continue with the rest as-is?

I think that makes sense - RHEL/CentOS 5 was the last major platform still running Python 2.4, and that went End-of-Life in March.

Yes. We have dropped almost all references to 2.x from 3.x docs (but the devguide is not versioned) and Nick confirmed my suspicion that a 2.4 reference is no longer needed for 2.x either. Not knowing the history, I found the current text it initially confusing.

terryjreedy

Remove ancient history irrelevant to current development.

terryjreedy · 2017-05-07T19:53:35Z

compiler.rst

+1. Parse source code into a parse tree (:file:`Parser/pgen.c`)
+2. Transform parse tree into an Abstract Syntax Tree (:file:`Python/ast.c`)
+3. Transform AST into a Control Flow Graph (:file:`Python/compile.c`)
+4. Emit bytecode based on the Control Flow Graph (:file:`Python/compile.c`)

 Starting with Python 2.5, the above steps are now used.  This change
 was done to simplify compilation by breaking it into three steps.


Delete these two lines.

DimitrisJim · 2017-05-08T04:30:12Z

Thanks for the review, Terry! I updated the Abstract to remove the historical reference.

willingc · 2017-05-08T06:10:10Z

compiler.rst

@@ -7,23 +7,14 @@ Design of CPython's Compiler
 Abstract
 --------

-Historically (through 2.4), compilation from source code to bytecode
-involved two steps:
+In CPython, the compilation from source code to bytecode involves three steps:


Shouldn't this be four steps as written?

I was confused by that too TBH. From what I understand, Brett (who originally wrote this from what I know) considered the last two as a single step because, as stated later:

The conversion process is initiated by a call to the function PyAST_Compile() in Python/compile.c . This function does both the conversion of the AST to a CFG and outputting final bytecode from the CFG.

Maybe a sub-list would do the trick? I'd like to wait for feed-back before doing it, though :-)

From the original, it should be 'involves four steps' and 'the latter three steps'. I intended those two phrasings to be kept.
Note 1. I am aware that this is not a general review of content, just of markup, but this opening was too annoying to ignore. I intentionally did not look at content further on ;-).
Note 2. I have not reviewed rst markup (have forgotten too much). Hope someone else will, and say so.

Note 2.

I've reviewed the rst markup as well as run the file through restructuredtext_lint. Looks good.

Feel free to just say "few" or "several" to avoid counting disagreements. 😉

Thanks for the review @willingc (nice tool, too!). I updated the text to use 'several'.

willingc · 2017-05-08T06:10:21Z

compiler.rst

-Starting with Python 2.5, the above steps are now used.  This change
-was done to simplify compilation by breaking it into three steps.
-The purpose of this document is to outline how the latter three steps
+The purpose of this document is to outline how these three steps


DimitrisJim · 2017-05-29T12:19:11Z

Anything else I need to address with this PR? (Bump)

serhiy-storchaka

Some information is outdated, but this can be changed by other PR.

serhiy-storchaka · 2017-06-01T04:52:24Z

compiler.rst

-numbers, etc.) are kept in Include/token.h.  The parse tree is made up
-of ``node *`` structs (as defined in Include/node.h).
+numbers, etc.) are kept in :file:`Include/token.h`.  The parse tree is made up
+of ``node *`` structs (as defined in :file:`Include/node.h`).

 Querying data from the node structs can be done with the following
 macros (which are all defined in Include/node.h):


Highlight Include/node.h?

serhiy-storchaka · 2017-06-01T04:57:50Z

compiler.rst

-stmt arguments for 'body', and zero or more expr arguments for
-'decorators'.
+Modifiers on the argument type specify the number of values needed; ``?``
+means it is optional, ``*`` means 0 or more while no modifier means only one


Is a comma before "while" needed?

serhiy-storchaka · 2017-06-01T05:05:38Z

compiler.rst

@@ -209,29 +201,29 @@ one is working with (e.g., if you are working with an 'if' statement
 you need to watch out for the ':' token to find the end of the conditional).

 The functions called to generate AST nodes from the parse tree all have
-the name ast_for_xx where xx is the grammar rule that the function
-handles (alias_for_import_name is the exception to this).  These in turn
+the name ``ast_for_xx`` where xx is the grammar rule that the function


Emphasize xx: *xx*. And in other cases below.

Maybe ast_for_xx can be formatted as: ``ast_for_``*xx*. But I don't know whether this works and how it looks.

serhiy-storchaka · 2017-06-01T05:06:37Z

compiler.rst

 that break the task down by various AST node types.  The functions are
-all named compiler_visit_xx where xx is the name of the node type (such
+all named ``compiler_visit_xx`` where xx is the name of the node type (such
 as stmt, expr, etc.).  Each function receives a ``struct compiler *``
 and xx_ty where xx is the AST node type.  Typically these functions


Highlight xx_ty.

serhiy-storchaka · 2017-06-01T05:10:12Z

compiler.rst

@@ -371,23 +361,24 @@ bytecode step of the compiler.  Several pieces of code throughout Python depend
 on having correct information about what bytecode exists.

 First, you must choose a name and a unique identifier number.  The official
-list of bytecode can be found in Include/opcode.h .  If the opcode is to take
+list of bytecode can be found in :file:`Include/opcode.h`.  If the opcode is to take


Highlight Include/opcode.h below?

serhiy-storchaka · 2017-06-01T05:11:52Z

compiler.rst

 will also need a new case statement for the new opcode in the big switch
-statement in PyEval_EvalFrameEx().
+statement in ``PyEval_EvalFrameDefault``.


_PyEval_EvalFrameDefault()

serhiy-storchaka · 2017-06-01T05:14:09Z

compiler.rst

-        Parser for ASDL definition files. Reads in an ASDL 
-	description and parses it into an AST that describes it.
+        Parser for ASDL definition files. Reads in an ASDL description
+        and parses it into an AST that describes it.


Highlight file paths below and remove spaces before periods.

serhiy-storchaka

Could you update the PR?

DimitrisJim · 2017-06-27T17:25:24Z

Sorry for the delay, I've been swamped. Will address review by the weekend.

serhiy-storchaka · 2017-07-03T12:36:25Z

Thank you for your PR @DimitrisJim. In the following PR I'll rewrite some outdated parts.

willingc · 2017-07-03T17:31:40Z

Thanks @DimitrisJim 🍰 and @serhiy-storchaka for reviewing and merging ☀️

…ython#192)

Use reST constructs in compiler.rst

0cce7e8

the-knights-who-say-ni added the CLA signed label May 7, 2017

terryjreedy reviewed May 7, 2017

View reviewed changes

terryjreedy requested changes May 7, 2017

View reviewed changes

Address review comments, clear 2.x references

dd20832

willingc reviewed May 8, 2017

View reviewed changes

Use 'several' steps

623ba2b

serhiy-storchaka reviewed Jun 1, 2017

View reviewed changes

serhiy-storchaka requested changes Jun 25, 2017

View reviewed changes

DimitrisJim added 2 commits July 2, 2017 01:59

Merge branch 'master' of github.com:python/devguide into issue181

e6963dc

Address review comments

1092bdd

serhiy-storchaka approved these changes Jul 3, 2017

View reviewed changes

serhiy-storchaka merged commit 384bb4b into python:master Jul 3, 2017

AA-Turner pushed a commit to AA-Turner/devguide that referenced this pull request Jun 17, 2022

Improve formatting and remove obsolete information in compiler.rst (p…

83ca8c1

…ython#192)

Uh oh!

Fix a number of issues with compiler.rst #192

Fix a number of issues with compiler.rst #192

Uh oh!

Conversation

DimitrisJim commented May 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

terryjreedy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DimitrisJim commented May 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DimitrisJim May 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DimitrisJim commented May 29, 2017

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

DimitrisJim commented Jun 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka commented Jul 3, 2017

Uh oh!

willingc commented Jul 3, 2017

Uh oh!

Uh oh!

DimitrisJim May 8, 2017 •

edited

Loading

DimitrisJim commented Jun 27, 2017 •

edited

Loading