simplified u.r - albeit not sure whether it can be called a simplification. It's shorter though. #45
Conversation
src/cforth/dot.fth
Outdated
| 0 base @ um/mod | ||
| ?dup if rot recurse else swap 0 max spaces then | ||
| digits + c@ emit ; | ||
|
|
There was a problem hiding this comment.
I like your code but please reformat according to my conventions, including running stack diagrams on the right. I'm a stickler about stylistic consistency within a code base.
| true ( true ) | ||
| else ( adr ) | ||
| find-position ( ) | ||
| ?dup 0= if |
There was a problem hiding this comment.
I prefer the origin code in this case as it seems clearer to me.
| : (indent) ( -- ) lmargin @ #out @ - 0 max spaces ; | ||
| : (indent) ( -- ) lmargin @ #out @ - | ||
| \ 0 max \ unnecessary, as spaces does that too. | ||
| spaces ; |
There was a problem hiding this comment.
Formatting: if the definition fits entirely on one line, it can follow the stack diagram. Otherwise the entire body must be on a 3-space indent and the ; is on a line by itself
| then | ||
| 2drop |
There was a problem hiding this comment.
This change seems hardly worth the effort. Your formulation compiles to slightly shorter code, but the improvement is so slight...
| @@ -142,7 +142,7 @@ variable break-type variable break-addr variable where-break | |||
| : bare-if? ( ip-of-branch-target -- f ) | |||
| /branch - /token - dup token@ ( ip' possible-branch-acf ) | |||
| dup ['] branch = \ unconditional branch means else or repeat | |||
| if drop drop false exit then ( ip' acf ) | |||
| if 2drop false exit then ( ip' acf ) | |||
There was a problem hiding this comment.
Okay but add another space before the then (fixing my formatting error).
| then ( adr,type ) | ||
| r> drop ( adr,type ) |
There was a problem hiding this comment.
I hope you aren't going to continue to swamp me with little tweaks of this magnitude...
| else | ||
| emit | ||
| emit |
There was a problem hiding this comment.
Thanks for fixing the tabs and trailing whitespace
| @@ -1,5 +1,6 @@ | |||
| : pluck ( x1 x2 x3 - x1 x2 x3 x1 ) 2 pick ; | |||
There was a problem hiding this comment.
Formatting: 2 spaces before closing ;
| : move ( from to len -- ) | ||
| -rot 2dup u< if rot cmove> else rot cmove then | ||
| pluck pluck u< if cmove> else cmove then |
| : ok ; | ||
| : trigger 0 0 2>r 2r> drop drop ; immediate | ||
| : trigger 0 0 2>r 2r> drop drop ; immediate \ what's that for? "magic" 0. return stack marker? |
There was a problem hiding this comment.
This was for triggering a logic analyzer in some scenario. I forget the details. It could go away.
|
Hi Mitch,
I apologize for all those little annoying changes which I'd never had
pushed as change request to you, agreeing with you that those are rather
irrelevant. Changes to mere formatting were caused by my editor
settings. I wasn't aware that those went into the original pushed change
request when I synced local repo with the forked repo. You're entirely
right to reject those.
Am Sat, 05 Oct 2019 01:17:51 -0700
schrieb Mitch Bradley <[email protected]>:
… MitchBradley requested changes on this pull request.
|
|
I tried to apply the recursive version of u.r, but it doesn't work because "recurse" is defined later in the load sequence. That version of dot is only used for debugging the build. Once the problem is found, dot.fth is omitted from the build and the real versions of u.r and friends are used. I haven't used it for a very long time. It is not even loaded by default - the line that loads it in basics.fth is commented out. My version doesn't work either, due to some relatively recent changes. I fixed my version just in case. I don't see much value in optimizing it since the code never appears in a final build. |
penalty is more use of return stack.