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

IMLAC: Type consistency warnings. #430

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

bscottm
Copy link
Contributor

@bscottm bscottm commented Sep 28, 2024

No description provided.

@@ -1,9 +1,9 @@
# Attach to a server. If the remote port isn't telnet, append ;notelnet.
attach tty 12345,connect=localhost:23
attach tty 12345,connect=192.168.50.248:23
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unintentional.

# Configure a bootstrap ROM; some programs require this.
set rom type=stty
# Load SSV version 22.
load -s ssv22.iml
load -s %~p0/ssv22.iml
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unintentional.

imlac/imlac_dp.c Outdated
sim_activate_after (&dp_unit, 2);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this change.

@@ -602,13 +605,13 @@ flag_check (uint16 flag)
}

static uint16
irq_iot (uint16 insn, uint16 AC)
irq_iot (uint16 insn, uint16 CUR_AC)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove these changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shadows AC -- is shadowing AC intentional?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The global AC should not be used inside this function.

Copy link
Contributor

@larsbrinkhoff larsbrinkhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm the original author of the Imlac simulator. I suggest some changes to be made to this pull request.

@markpizz
Copy link
Contributor

@larsbrinkhoff given the set of revisions to the original change, why not merely consider what's left of the original changes as a "suggestion" and submit a new PR which is exactly what you want, with comments referring to the original suggestion if you want.

@larsbrinkhoff
Copy link
Contributor

@markpizz I think updating this pull request should suffice. History rewriting and a force push will do this.

@bscottm
Copy link
Contributor Author

bscottm commented Oct 1, 2024

I pushed the individual change sets so that it is clear what was done. Paul can squash the individual commits into one big comment when the PR is merged.

Copy link
Contributor

@larsbrinkhoff larsbrinkhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now. Thanks!

@bscottm
Copy link
Contributor Author

bscottm commented Oct 3, 2024

@larsbrinkhoff: Fewer warnings declutters the build output. Appreciate you indulging my changes.

@larsbrinkhoff
Copy link
Contributor

Maintainers, @pkoning2 et al, I recommend squash merging these commits.

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.

3 participants