-
-
Notifications
You must be signed in to change notification settings - Fork 29
Modify CL:CHAR-NAME always to return string from lookup in IL:CHARACTERNAMES #1377
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
Conversation
…ERNAMES. This *seems* to fix the issue with printing "control characters" in #\Name format.
# Resolved Conflicts: # sources/CMLCHARACTER # sources/CMLCHARACTER.LCOM
nbriggs
left a comment
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.
Fix in CL:CODE-NAME Looks good - but why is file CMLARITH in this branch? It's not involved in the fix and therefore I don't see why we should be changing it.
|
Ah, now I see your comment about CMLARITH - I guess that's OK, though it is not the way I would do it. |
|
The original branch was just about updating the filemaps—I noticed that both those files could use it.
Then we fell into this atom/string problem.
… On Oct 30, 2023, at 6:19 PM, Nick Briggs ***@***.***> wrote:
@nbriggs requested changes on this pull request.
Fix in CL:CODE-NAME Looks good - but why is file CMLARITH in this branch? It's not involved in the fix and therefore I don't see why we should be changing it.
—
Reply to this email directly, view it on GitHub <#1377 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AQSTUJLF7DW7BWQSIEOE2PDYCBG3RAVCNFSM6AAAAAA6W3LRY6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOMBVGI4TKNBZGY>.
You are receiving this because your review was requested.
|
|
This is essentially the IL:MKSTRING solution, it makes the error go away at the cost of creating a new string pointer but not a new pname) on each call if IL:CHARACTERNAMES is, as specified in IRM, an alist with litatom CAR's. So if this is an inner-loop function, there may be a performance issue, as we talked about. But maybe that is small compared to all the other overhead of simply outputting a character. |
|
@rmkaplan As I stated in the comment above, the internal data structure for a As far as any additional performance hit, the potential call to Also, technically |
|
My recollection is that the pname of a litatom/symbol is a sequence of the same sort as the characters of a string (an ALLOCBLOCK). When a string is made out of a litatom, a string pointer is created that points exactly to that block. But that particular string pointer is special in that it is marked as read-only. The primitive operations that can modify the characters of a string all check that, and copy into a new block before making any change, so that the atom pname remains intact.
I don’t have a sense of when or how often this is called. I got the impression in the meeting today that it was called quite often, but I guess that’s not the case.
… On Oct 30, 2023, at 11:20 PM, Matt Heffron ***@***.***> wrote:
@rmkaplan <https://github.com/rmkaplan> As I stated in the comment above, the internal data structure for a LITATOM (same as LISP:SYMBOL) doesn't store the pname as a STRING, all functions that get the pname for a LITATOM create a new string instance.
As far as any additional performance hit, the potential call to LISP:STRING is only for characters in the IL:CHARACTERNAMES list, which are extremely unlikely to occur.
Also, technically IL:CHARACTERNAMES isn't a true ALIST, because the character-code is not the CDR of the list entry, but it's the CADR. I suppose that means that the STRING representation of the name could be placed in the CADDR (or CDADR) of the list entry, and only require using the LISP:STRING function if that value wasn't present.
—
Reply to this email directly, view it on GitHub <#1377 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AQSTUJMMUQZCK2ODWCA35UDYCCKBHAVCNFSM6AAAAAA6W3LRY6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOBWGUYTSMZRGU>.
You are receiving this because you were mentioned.
|
|
I haven't found any inner-loop uses of |
This seems to fix the issue (#1375) with printing "control characters" in
#\Nameformat.It appears to be impossible to get the canonical
STRINGrepresenting theNAMEof aLITATOM/SYMBOL(without itsPACKAGE). That isn't stored anywhere as aSTRING. A newSTRINGis created every time.However,
(STRING "foo")will return the same instance of"foo", so it doesn't create a newSTRINGunless required.This commit was on top of the recent
masterwithRemake-CMLARITH-filemapmerged (tag: medley-231029-e2d8c9e5).I also deleted the .DFASL, and forcibly compiled to .LCOM.