Skip to content

Conversation

@serialhex
Copy link
Contributor

@serialhex serialhex commented Oct 20, 2020

1120-1129 of #217

A couple of notes while reviewing my PR:

White space

I tried to match the indentation and white space exactly, which means that many paragraphs begin with \t (3 spaces). You can see this when looking at line 420 of the file, and comparing it to page 1120 in the Comanche scans. The leading "S" is over the "D" in the following line, and looking down, over the "L" in the word "LAST" for the code.

The entire file seems to follow this convention, but while the previous 419 lines are said to have been proofed, this is not reflected in the file. Looking back 1 page, and at line 387, it should be indented, and is not. Just giving reviewers a heads-up and if you want my version reverted I will. If you want the rest of the file changed to match let me know and I'll make another PR with those changes.

Also, there is some indentation-fiddly-ness that exists starting on line 551; page 1125, and the same thing happens starting on 650; page 1128. Tabs followed by spaces to get letters to line up properly.

Bad code?

Starting at line 662; page 1128 there is this code:

		# Was CAF --- RSB 2009.
		CA	0		# GET BASE ADDRESS OF CADR LIST.

If you look at the scan though, the code should be:

		CAF	0		# GET BASE ADDRESS OF CADR LIST.

I don't know enough about AGC assembler to know if this is a bug that was fixed and needs to remain different from the scans, or if someone thought it was a bug, changed it, and now the running code doesn't match the scans and the original. I am more than happy to change it to match the scan, but I didn't want to change a bugfix and potentially reintroduce such a thing to the system (as there may be people running this code).

Thank you for your time, and please let me know if there are any changes that should be made.

@wopian wopian self-assigned this Oct 20, 2020
@wopian wopian self-requested a review October 20, 2020 20:32
@wopian wopian added Status: Review Needed Type: Proof Comanche55 and Luminary99 files labels Oct 20, 2020
@wopian wopian added this to the Comanche055 milestone Oct 20, 2020
@wopian wopian mentioned this pull request Oct 22, 2020
@wopian
Copy link
Collaborator

wopian commented Oct 27, 2020

I've added the hacktoberfest-accepted label to ensure your contribution counts towards Hacktoberfest, as I haven't been able to review as many PRs as I had hoped over the past few days during my free time.

PRs are being throughly reviewed from oldest to newest 👍


I'm on mobile at the moment, but have set a reminder and will get back to you on the spacing/bad code tomorrow before I review the earlier PRs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Status: Review Needed Type: Proof Comanche55 and Luminary99 files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants