racket / ChezScheme

Chez Scheme
Apache License 2.0
110 stars 8 forks source link

Adds hashtable-ref-cell #12

Closed 97jaz closed 5 years ago

97jaz commented 5 years ago

... and eq-hashtable-cell and symbol-hashtable-ref-cell, which are just like hashtable-cell, except that if the given key isn't present, they return #f instead of mutating the table.

(Of course, my purpose in proposing this is to make hash-ref-key more efficient on racketcs, but I'm not at all sure it's worth it if it means deviating from upstream even more. Should I propose this to the upstream repo? Should I just not bother at all?)

mflatt commented 5 years ago

So far, it has been manageable to prioritize adding things we want over staying closer to upstream.

We should keep the docs up-to-date with changes, though. Would you prefer to update (in "csug") or to merge this change and then I'll update the docs?

97jaz commented 5 years ago

Running into a problem with csug. I added the new documentation, but when I make docs, it fails, saying Exception in list-libraries: no libraries for hashtable-ref-cell defined. I understand why the error occurs; I added hashtable-ref-cell to a section of primdata.ss that doesn't name any libraries:

(define-symbol-flags* ([libraries] [flags primitive proc])
  ...)

The part I don't understand is that hashtable-cell is listed in this same section but does not suffer from the same problem.

mflatt commented 5 years ago

Commit d42c9ab hashashtable-ref-cell in same place as hashtable-ref, instead of the same place as hashtable-cell. Moving it solves the problem for me. Did you already make that change to "primdata.ss"?

97jaz commented 5 years ago

Ugh. Nope, my mistake. (eq-hashtable-ref-cell and symbol-hashtable-ref-cell were in the right place, but not hashtable-ref-cell.) Thanks!

97jaz commented 5 years ago

Ok, docs updated.

Also, I noticed that the tests failed on the previous run, because the mats summary is, it seems, a diff against expected data. Am I supposed to add the expected output somewhere?

mflatt commented 5 years ago

Yes, that's the next bit of trouble. The solution involves first running the test suite, making sure the new results are what you want, and then using "errors-compile-0-f-f-f" and "errors-compile-3-f-f-f" as replacements for "root-experr-compile-0-f-f-f" and "root-experr-compile-3-f-f-f". Also make patches, or something like that. I'm still not yet clear on the right sequence and make targets.

97jaz commented 5 years ago

I followed the directions here, but I'm still stumped.

First off, though, when I did make allx, at the end I saw:

check heap detected errors---grep standard output for !!!
make[2]: *** [all] Error 255
make[1]: [allxhelp] Error 2 (ignored)

As you can see, the error was ignored, and make went on to update the report, but it was concerning. The scrollback buffer contained stuff like:

compiling testfile.ss with output to testfile.so
Chez Scheme Version 9.5.3
Copyright 1984-2019 Cisco Systems, Inc.

> compiling testfile.ss with output to testfile.so
> 
***** expect transcript output:
Chez Scheme Transcript [Fri Jul 26 14:42:54 2019]
OK, #<void>
!!! (check_dirty): unexpected space 11 for dirty segment 0x5269c
!!! (check_dirty): unexpected space 11 for dirty segment 0x5269c
!!! Unnecessary dirty byte 1 (ff) after gc for segment 0x5269c card 6 segment 0x5269c generation=4 space-ip-tobj
!!! (check_dirty): unexpected space 11 for dirty segment 0x5269c
!!! (check_dirty): unexpected space 11 for dirty segment 0x5269c
!!! Unnecessary dirty byte 1 (ff) after gc for segment 0x5269c card 6 segment 0x5269c generation=4 space-ip-tobj

... and so forth. I don't know if that's expected or not, but it reads like the GC is doing surprising things. I seriously doubt that's related to my changes, but it seems worth mentioning.

At any rate, after I did make patches and make just-reports the reports files were indeed empty, but git doesn't report any changes, because they're all within the machine architecture-specific workspace directory, which is ignored by git. So I assume I need to copy something somewhere, but I have absolutely no idea what.

Am I supposed to copy the root-experr-* and patch- files from the workspace to the main mats directory?

mflatt commented 5 years ago

Yes, that's right: copy them from the work area to the main "mats" directory.

The error with space 11 is probably because I didn't update some debugging support when adding object-backreferences. I'll look into that sometime, and you can certainly ignore it for now.

97jaz commented 5 years ago

Okay, @mflatt, I think this is ready to go. Thanks for your help with the mats.

mflatt commented 5 years ago

Looks like we to make sure Racket sources are recompiled after this kind of change to Chez Scheme, which means bumping the version number and adding a check to "racket/src/cs/compile-file.ss". I'll do that.