-
-
Notifications
You must be signed in to change notification settings - Fork 27
FONTSAMPLE add option for which charset to display #2002
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
Closed
MattHeffron
wants to merge
2
commits into
master
from
mth32--FONTSAMPLE-Add-option-for-charset-to-display
Closed
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Binary file not shown.
Binary file not shown.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes me uncomfortable to introduce a dependency on "LOADUPSDIRECTORIES" - if it really needs EXPORTS.ALL (does it?) then just (FILES EXPORTS.ALL) and it should use DIRECTORIES which should be set up in the site init file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, in general, how should dependency on system level definitions (e.g.,
CHARSETINFO,FONTDESCRIPTOR, ...) be set in theCOMSfor modules in LIBRARY or LISPUSERS? The pattern:(DECLARE%: EVAL@COMPILE DONTCOPY (FILES (FROM LOADUPS) EXPORTS.ALL))I found in
UNICODECOMS. (@rmkaplan)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the convention in the past was that those working on the "low level" system or calls to it would load the file SYSEDIT; SYSEDIT sets some variables as well as loading EXPORTS.ALL if needed. (There was also a file called "ABC" which looked like SYSEDIT and I tried to merge the two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See also related discussion in PR 2003
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@masinter
But ... SYSEDIT does:
(RESETVARS ((CROSSCOMPILING T)) (FILESLOAD (SOURCE FROM LOADUPS) EXPORTS.ALL))So why wouldn't it be acceptable for individual modules (e.g., on lispusers) to do a similar
FILESLOADunder(DECLARE%: EVAL@COMPILE DONTCOPY. (Without the(RESETVARS ((CROSSCOMPILING T))which is SYSEDIT-specific.) This also wouldn't change other system-levelVARSthat a particular user might not want changed.More discussion? @rmkaplan @nbriggs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MattHeffron it only does it exactly like that because Larry changed it to do the FILESLOAD "FROM LOADUPS". It's the "FROM LOADUPS" that I'm objecting to, rather than having the right things on DIRECTORIES and not the fact of loading exports.all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But ... where did
LOADUPSDIRECTORIEScome from?I take it that you're proposing that it be removed from the standard loadup sysouts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It (
LOADUPDIRECTORIES) is set in sources/MEDLEYDIR -- and while I'm happy to have the mechanism there (though I think MEDLEYDIR should be in library not sources), I don't believe that that is the right place to set up a lot of values that are not actually a fundamental part of the system - that's the role of the site init (site greet) file.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, instead of a
LOADUPDIRECTORIESvariable, would it be better (acceptable) for individual modules (e.g., on lispusers) to have:Should the location of EXPORTS.ALL be a known place?
The loadup sysout files are in the loadups directory because the MEDLEYDIR/scripts/medley/medley.command expects to find them in loadups.
Why not have the same a priori condition that EXPORTS.ALL is in the same place?
Or, should modules like these have a MODULENAME-SETUP-TO-COMPILE function that does the setup, or prints the setup instructions?
And, is it generally better for modules to load EXPORTS.ALL (somehow), or should they use
LOADCOMPof the specific system module(s) needed? E.g., TEDIT has:(DECLARE%: EVAL@COMPILE DONTCOPY (FILES (LOADCOMP) UNICODE))I'm not trying to be difficult!
Should this discussion be extracted into a GitHub Issue or Discussion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it should just have
(DECLARE: EVAL@COMPILE DONTCOPY (FILES EXPORTS.ALL))- MEDLEYDIR and anything like that has anything to do with running on the emulator, unless the module is specifically targeted to something Unixy, should not appear in the module. The locations for everything should be set up in the site greet file and appear on DIRECTORIES, which FILESLOAD and friends already know about. I think that an EXPORTS.ALL that corresponds to the files that are insourcesandlibraryshould be checked in tolibrary, or at least it should appear there in the assets of the release. I don't know about TEDIT and UNICODE.