Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix broken line length in xt_words #271

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wkjagt
Copy link

@wkjagt wkjagt commented Oct 13, 2022

What is broken

words output formatting is broken, because the lines it outputs are too long:

image

The fix in this PR:

xt_words doesn't respect MAX_LINE_LENGTH correctly because at the end of each line it resets the length counter to 0, while the next word is already in memory to be printed on the next line. So effectively, for each line after the first, it starts counting the line length from 0 after printing the first word on that line. Instead it needs be set to the length of that first word (plus 1, for a space) so it's taken into consideration when the count continues after that word. This commit resets the counter to the length of the word already in memory (the first word to be printed on the new line), and increments it once to account for the space printed after the word. The length of that word is still on the stack at this point, put there by xt_name_to_string, so it can be taken directly from there with lda 0,x, exactly like the routine already does right above when adding the length of the word to the counter.

Output after the fix:

image

Fixes #270

xt_words didn't respect MAX_LINE_LENGTH because at the end of each line
it set the length counter to 0. Instead it should be set to the word
about to be printed because that's already in memory so its length
needs to be taken into consideration. This commit fixes that behaviour
and also adds one extra character to the count to account for the added
space.
@SamCoVT
Copy link
Contributor

SamCoVT commented Nov 15, 2022

Hi wkjagt - I just wanted to give you an update that Tali Forth 2 development will be moving over to my repository shortly as I am taking over development from Scot. I need to get my repository synced up with Scot's and then I will incorporate your patch (hopefully by this weekend). Thanks for issuing a pull request and for such a clear description of the issue.
Once I have my repository in order, you'll probably want to make it your upstream repo so you can get fixes for any future bugs/issues that are reported.

@SamCoVT
Copy link
Contributor

SamCoVT commented Nov 16, 2022

Hi wkjagt - I have my repository all set up and have already pulled this pull request through to my repository and merged it. You will want to change your upstream to SamCoVT/TaliForth2/master-ophis if you'd like to stay with the Ophis assembler or master-64tass (preferred) if you can handle switching to the 64tass assembler. I've merged your pull request in both places.
The latter is where any future development is expected to take place. The 64tass assembler has conditional assembling and allows wordsets you are not using to be skipped, making the ROM smaller.
This pull request might not be closed, as I don't have permissions here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

xt_words doesn't respect max line length correctly
2 participants