Skip to content

reset defaultexternalformat when returning#988

Closed
masinter wants to merge 1 commit intomasterfrom
unixcomm-reset-defaultexternal
Closed

reset defaultexternalformat when returning#988
masinter wants to merge 1 commit intomasterfrom
unixcomm-reset-defaultexternal

Conversation

@masinter
Copy link
Copy Markdown
Member

@masinter masinter commented Oct 11, 2022

Returning to previous saved image (after LOGOUT, SYSOUT, MAKESYS, SAVEVM), reset *NEW-SHELL-DEVICE*'s DEFAULTEXTERNALFORMAT to (SYSTEM-EXTERNALFORMAT).

Note that the current code has two devices *SHELL-DEVICE* and *NEW-SHELL-DEVICE* with a comment (1990) that some future release might only use *NEW-SHELL-DEVICE*. That change will be made later.

@masinter masinter requested a review from rmkaplan October 11, 2022 16:51
@nbriggs
Copy link
Copy Markdown
Contributor

nbriggs commented Oct 11, 2022

*SHELL-DEVICE* vs *NEW-SHELL-DEVICE* is about coordinating with the version of maiko and the sub-functions that the unixcomm.c code implements for pipes, sockets, process communication etc. We're definitely on the new version now, but having had to look some at the code in unixcomm.c, it's compensating for some poor choices in the Lisp code. We should probably review that interaction at some point in the future.

@masinter
Copy link
Copy Markdown
Member Author

this PR just does the minimum to effect the updating. I'm not so happy with the means of determining the external format of process streams -- scanning environment variables is ugly and likely wrong for some -- but we got here. Perhaps the external format should be determined by the subrs that are opening the stream?

@masinter masinter requested review from fghalasz and nbriggs October 13, 2022 23:00
@nbriggs
Copy link
Copy Markdown
Contributor

nbriggs commented Oct 14, 2022

I don't think the SUBRs know any better than the Lisp code what the text encoding is (at least by default) -- the code can override defaults by setting values in the environment but otherwise it's left looking at exactly the same environment variables that Lisp would look at, and it would be much harder for the C code to smash an external format into the stream than for the Lisp code to do it. There's a C structure matching the Lisp STREAM datatype, but I don't know that it knows about the external format because that's probably beyond the fields it currently needs to know.

@nbriggs
Copy link
Copy Markdown
Contributor

nbriggs commented Oct 14, 2022

... and having the SUBR or the Lisp code set the encoding behind the user's back would, if it's not really careful, make it impossible for Lisp code that created a shell stream to set the encoding in the environment to override the defaults.

@masinter
Copy link
Copy Markdown
Member Author

In this case, the default external format doesn't affect existing streams or streams opened with an explicit encoding. It couldn't anyway, because open process/socket/shell streams are closed when starting a saved VM.

@rmkaplan
Copy link
Copy Markdown
Contributor

Still poking, it now seems that WHEREIS doesn't work. Even though there whereis-hash is open.

@masinter
Copy link
Copy Markdown
Member Author

Did you pick up #1003 ?

Copy link
Copy Markdown
Contributor

@rmkaplan rmkaplan left a comment

Choose a reason for hiding this comment

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

I don't understand the NEW vs OLD devices, but at least it is consistent. Otherwise looks good, except there is a change from FAKE-COMPILE-FILE to CL:COMPILE-FILE. Which suggests that there should be a DFASL and that the LCOMs should be removed.

@masinter
Copy link
Copy Markdown
Member Author

IL:COMPILE-FILE is equivalent to FAKE-COMPILE-FILE as per PF* COMPILE-FILE?

Not sure of the history of that.

@masinter masinter requested a review from rmkaplan October 26, 2022 05:22
@masinter
Copy link
Copy Markdown
Member Author

I think this PR is subsumed by PR #1006 which gets rid of the "old" device in favor of one "new" one.
I'm not sure if there will be a conflict, but it won't be too hard to resolve if there is.

@rmkaplan
Copy link
Copy Markdown
Contributor

rmkaplan commented Oct 27, 2022 via email

@masinter
Copy link
Copy Markdown
Member Author

image

@nbriggs
Copy link
Copy Markdown
Contributor

nbriggs commented Oct 27, 2022

Ah... IL:COMPILE-FILE? not IL:COMPILE-FILE

@masinter
Copy link
Copy Markdown
Member Author

i'm puzzled by FAKE-COMPILE-FILE and why it was invented and when should it be used, and why 'FAKE:'?
I was looking for something else and stumbled on this. I don't remember changing the property but perhaps I did to see if it was equivalent.
Maybe we should make a list of questions for people who might know but aren't part of the group?

@nbriggs
Copy link
Copy Markdown
Contributor

nbriggs commented Oct 27, 2022

Between versions 40 and 42, UNIXCOMM acquired a FILETYPE property of FAKE-COMPILE-FILE. I don't know why.

@masinter
Copy link
Copy Markdown
Member Author

in commit 32128f5 UNIXCOMM;40 Jun 22 I noted

  • UNIXCOMM compile to DFASL; only set UTF-8 if getenv(LANG). ...

In commit d7ca40e @rmkaplan noted

  • UNIXCOMM: Default format comes from device
    Also, I seemed to have reverted back to LCOM with FAKE-COMPILE-FILE

I think what might have happened is that I thought I was setting it to make FASLs but put FILETYPE COMPILE-FILE instead of CL:COMPILE-FILE and didn't notice.
I'd recommend adding a "compiled file matches FILETYPE" to the PRC "sanity/version match" check.

The "why?" i was wondering was "why FAKE-COMPILE-FILE at all?".

This PR is followed by PR #1006 (assumed by?); shouldn't prc show the relationship?

@masinter
Copy link
Copy Markdown
Member Author

masinter commented Dec 7, 2022

I think this PR is subsumed by PR #1006 which gets rid of the "old" device in favor of one "new" one. I'm not sure if there will be a conflict, but it won't be too hard to resolve if there is.

Resolving the conflict: close this PR.

@masinter masinter closed this Dec 7, 2022
@rmkaplan
Copy link
Copy Markdown
Contributor

rmkaplan commented Dec 7, 2022 via email

@masinter
Copy link
Copy Markdown
Member Author

I think if you have newer version(s) of file(s) that you could check out a new branch with differences you're not sure of, and then just ask for review.
The QUOTE => Function change only happens in the UNIXCOMCOMS which Masterscope currently doesn't analyze.

@masinter masinter deleted the unixcomm-reset-defaultexternal branch February 23, 2023 04:13
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