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 unbound vars errors in WRITESTRIKEFONTFILE from earlier edit. #2003

Merged
merged 2 commits into from
Feb 3, 2025

Conversation

MattHeffron
Copy link
Contributor

No description provided.

@MattHeffron MattHeffron self-assigned this Feb 1, 2025
@MattHeffron MattHeffron added the bug Something isn't working (as per documentation) label Feb 1, 2025
@rmkaplan
Copy link
Contributor

rmkaplan commented Feb 1, 2025 via email

@MattHeffron
Copy link
Contributor Author

Moved those two constants from EDITFONT to FONT, per @rmkaplan.

@rmkaplan
Copy link
Contributor

rmkaplan commented Feb 2, 2025

I think the constants BITSPERWORD and BYTESPERWORD should also be removed from EDITFONT. Those are in MODARITH and EXPORTS.ALL

@MattHeffron
Copy link
Contributor Author

I think the constants BITSPERWORD and BYTESPERWORD should also be removed from EDITFONT. Those are in MODARITH and EXPORTS.ALL

So, if removed, should EDITFONT add MODARITH to the LOADCOMP? I.e., do

(DECLARE%: EVAL@COMPILE DONTCOPY
           (FILES (LOADCOMP) FONT MODARITH))

At this point, this is no longer part of fixing WRITESTRIKEFONTFILE in file FONT, but is more general clean up.

@rmkaplan
Copy link
Contributor

rmkaplan commented Feb 3, 2025 via email

@MattHeffron
Copy link
Contributor Author

No, EDITFONT should load EXPORTS.ALL for compiling, that brings in the constants from MODARITH.

See the discussion on PR 2002 that strongly implies not to automatically load EXPORTS.ALL, even under (DECLARE%: EVAL@COMPILE DONTCOPY .
Loading EXPORTS.ALL here makes the LOADCOMP of FONT superfluous.

How do we want these kind of dependencies to be "taken care of"?
Precisely, with LOADCOMP, like EDITFONT is currently?
Or wholesale, by loading EXPORTS.ALL?
Or, manually as required, by loading SYSEDIT?

Copy link
Contributor

@nbriggs nbriggs left a comment

Choose a reason for hiding this comment

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

Approved per Monday Feb 3 2025 meeting.

@MattHeffron MattHeffron merged commit 40d18ff into master Feb 3, 2025
@masinter masinter deleted the mth31--Fix-WRITESTRIKEFONTFILE-unbound-vars branch February 3, 2025 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working (as per documentation)
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants