formatting details of 5.5 #443
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
| } else if (target !== "val" && linkage === "return") { | ||
| error(target, "return linkage, target not val -- compile"); | ||
| } else {} | ||
| const return_undefined = make_label("return_undefined"); |
There was a problem hiding this comment.
Very nice rewrite! Happy to get rid of the empty else. I'll see if the same trick can be applied elsewhere (think there is one empty else elsewhere).
Only one minor negative: this will cause labels to be computed even when they are not used, which will lead to gaps in the sequences of label numbers. But very minor thing.
| & \texttt{restore(}\textit{reg}_1\texttt{)} & \texttt{restore(}\textit{reg}_2\texttt{)} & \textit{seq}_1 \\ | ||
| & \textit{seq}_2 & \textit{seq}_2 & \texttt{restore(}\textit{reg}_1\texttt{)} \\ | ||
| & & & \texttt{restore(}\textit{reg}_2\texttt{)} \\ | ||
| \textit{seq}_1 & \texttt{save(}\textit{reg}_1\texttt{),} & \texttt{save(}\textit{reg}_2\texttt{),} & \texttt{save(}\textit{reg}_2\texttt{),} \\ |
There was a problem hiding this comment.
Is it not too much with both the | and the comma after?
There was a problem hiding this comment.
Also, a bit strange then not to use comma consistently (not after
| </SCHEME> | ||
| <JAVASCRIPT> | ||
| component applications whose function expression is a name, | ||
| components applications whose function expression is a name, |
There was a problem hiding this comment.
Is this really grammatically correct? Do we pluralise both words?
| avoids compiling the | ||
| <QUOTE>dead code</QUOTE> after the return statement that can | ||
| never be executed. Not compiling dead<FOOTNOTE>Our compiler does not detect all dead code. For example, | ||
| never be executed. Not compiling dead code<FOOTNOTE>Our compiler does not detect all dead code. For example, |
There was a problem hiding this comment.
You OK with the use of "code" here? I thought about it after our recent email conversation. "Dead code" can be Googled.
We could call them "dead instructions".
| list("argl"), | ||
| list(assign("argl", list(op("pair"), | ||
| reg("val"), reg("argl")))))); | ||
| const code_for_next_arg = |
There was a problem hiding this comment.
Let's go with this reformat for now -- but (I'm just echoing Julie here) we will have quite a bit of horizontal space so vertical space is a more scarce resource. Again, expecting to have to revisit all formatting of code when typesetting the book.
| Hi Tobias: See inline below. -martin http://www.comp.nus.edu.sg/~henz On Sun, 24 Jan 2021 at 22:03, Tobias Wrigstad ***@***.***> wrote: ***@***.**** commented on this pull request. Some questions in the comments. I am happy to do the edits afterwards. You're already going above and beyond here. ------------------------------ In xml/chapter5/section5/subsection3.xml <#443 (comment)>: > - fun_return, - assign(target, reg("val")), - go_to(label(linkage)))); - } else if (target === "val" && linkage === "return") { - return make_instruction_sequence( - list("fun"), - all_regs, - list( - save("continue"), - save_marker(), - assign("val", list(op("compiled_function_entry"), reg("fun"))), - go_to(reg("val")))); - } else if (target !== "val" && linkage === "return") { - error(target, "return linkage, target not val -- compile"); - } else {} + const return_undefined = make_label("return_undefined"); Very nice rewrite! Happy to get rid of the empty else. I'll see if the same trick can be applied elsewhere (think there is one empty else elsewhere). Only one minor negative: this will cause labels to be computed even when they are not used, which will lead to gaps in the sequences of label numbers. But very minor thing. I see. Minor thing, I'd say. ------------------------------ In xml/chapter5/section5/subsection1.xml <#443 (comment)>: > @@ -448,11 +448,11 @@ go_to(reg("continue")), \[ { \begin{array}{l|l|l|l} -\textit{seq}_1 & \texttt{save(}\textit{reg}_1\texttt{)} & \texttt{save(}\textit{reg}_2\texttt{)} & \texttt{save(}\textit{reg}_2\texttt{)} \\ -\textit{seq}_2 & \textit{seq}_1 & \textit{seq}_1 & \texttt{save(}\textit{reg}_1\texttt{)} \\ - & \texttt{restore(}\textit{reg}_1\texttt{)} & \texttt{restore(}\textit{reg}_2\texttt{)} & \textit{seq}_1 \\ - & \textit{seq}_2 & \textit{seq}_2 & \texttt{restore(}\textit{reg}_1\texttt{)} \\ - & & & \texttt{restore(}\textit{reg}_2\texttt{)} \\ +\textit{seq}_1 & \texttt{save(}\textit{reg}_1\texttt{),} & \texttt{save(}\textit{reg}_2\texttt{),} & \texttt{save(}\textit{reg}_2\texttt{),} \\ Is it not too much with both the | and the comma after? Well, the comma is needed I feel, because that's how the instructions are separated in our setup. ------------------------------ In xml/chapter5/section5/subsection1.xml <#443 (comment)>: > @@ -746,7 +751,7 @@ f(g("x"), "y") expressions combinations whose operator is a symbol, </SCHEME> <JAVASCRIPT> - component applications whose function expression is a name, + components applications whose function expression is a name, Is this really grammatically correct? Do we pluralise both words? You are mistaken. SICP has the same construction and is correct. I had to read it a couple of times to understand the meaning. Try it. Hint: make a pause between the two words. ------------------------------ In xml/chapter5/section5/subsection2.xml <#443 (comment)>: > @@ -748,10 +747,10 @@ function compile_sequence(seq, target, linkage) { Treating return statements as if they always appear as the last statement in a sequence avoids compiling the <QUOTE>dead code</QUOTE> after the return statement that can - never be executed. Not compiling dead<FOOTNOTE>Our compiler does not detect all dead code. For example, + never be executed. Not compiling dead code<FOOTNOTE>Our compiler does not detect all dead code. For example, You OK with the use of "code" here? I thought about it after our recent email conversation. "Dead code" can be Googled. We could call them "dead instructions". "dead code" is good. Can make an exception with this one, haha. ------------------------------ In xml/chapter5/section5/subsection3.xml <#443 (comment)>: > @@ -434,14 +434,15 @@ function construct_arglist(arg_codes) { } function code_to_get_rest_args(arg_codes) { - const code_for_next_arg = preserving( - list("argl"), - head(arg_codes), - make_instruction_sequence( - list("val", "argl"), - list("argl"), - list(assign("argl", list(op("pair"), - reg("val"), reg("argl")))))); + const code_for_next_arg = Let's go with this reformat for now -- but (I'm just echoing Julie here) we will have quite a bit of horizontal space so vertical space is a more scarce resource. Again, expecting to have to revisit all formatting of code when typesetting the book. OK. ------------------------------ In xml/chapter5/section5/subsection1.xml <#443 (comment)>: > @@ -448,11 +448,11 @@ go_to(reg("continue")), \[ { \begin{array}{l|l|l|l} -\textit{seq}_1 & \texttt{save(}\textit{reg}_1\texttt{)} & \texttt{save(}\textit{reg}_2\texttt{)} & \texttt{save(}\textit{reg}_2\texttt{)} \\ -\textit{seq}_2 & \textit{seq}_1 & \textit{seq}_1 & \texttt{save(}\textit{reg}_1\texttt{)} \\ - & \texttt{restore(}\textit{reg}_1\texttt{)} & \texttt{restore(}\textit{reg}_2\texttt{)} & \textit{seq}_1 \\ - & \textit{seq}_2 & \textit{seq}_2 & \texttt{restore(}\textit{reg}_1\texttt{)} \\ - & & & \texttt{restore(}\textit{reg}_2\texttt{)} \\ +\textit{seq}_1 & \texttt{save(}\textit{reg}_1\texttt{),} & \texttt{save(}\textit{reg}_2\texttt{),} & \texttt{save(}\textit{reg}_2\texttt{),} \\ Also, a bit strange then not to use comma consistently (not after $seq_1$). Well, a sequence is a sequence and not an individual instruction. I think the absence of the comma clarifies the structure, rather than confusing it. … — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#443 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHGSDYARGW7DT5LOSOJORZTS3QSDHANCNFSM4WQLQPYA> . |
Indeed. Finally got it. :) |
TobiasWrigstad approved these changes Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
No description provided.