Skip to content

Improve Tcl 8.7/9.0 readiness, const usage #42

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

Merged
merged 9 commits into from
Mar 25, 2023

Conversation

chrstphrchvz
Copy link
Contributor

Tcl_GetObjType() returns const as of Tcl 8.6, and Tcl.xs never modifies these structs

@chrstphrchvz
Copy link
Contributor Author

chrstphrchvz commented Apr 22, 2022

In order to test changes in Tcl::pTk for supporting Tk 8.7, I would like get Tcl.pm to build and run with Tcl 8.7/9.0. I am going to repurpose this pull request to include other changes which I believe will be needed:

  • Tcl.xs assumes tcl.h always defines the STRINGIFY()/JOIN() macros, but in Tcl 9.0 (and Tcl 8.7 with -DTCL_NO_DEPRECATED) they are normally not defined:
    /usr/lib/perl5/5.34/core_perl/CORE/perl.h:4904:33: error: expected ‘)’ before ‘STRINGIFY’
     4904 | #define PERL_API_VERSION_STRING STRINGIFY(PERL_API_REVISION) "." \
          |                                 ^~~~~~~~~
    
    The definitions of STRINGIFY()/JOIN() in tcl.h have been guarded by #ifndef since Tcl 8.4b2, and identical to the definitions in Perl. I believe it is okay for Tcl.xs to rely on Perl's definitions for them and not #undef them before including tcl.h.
  • Due to Tcl TIP 543, tcl.h no longer defines TCL_INTERP_DESTROYED in Tcl 9.0 (and Tcl 8.7 with TCL_NO_DEPRECATED defined):
    Tcl.xs:1873:56: error: ‘TCL_INTERP_DESTROYED’ undeclared (first use in this function); did you mean ‘TCL_TRACE_DESTROYED’?
     1873 |         newCONSTSUB(stash, "INTERP_DESTROYED", newSViv(TCL_INTERP_DESTROYED));
          |                                                        ^~~~~~~~~~~~~~~~~~~~
    
    Existing code relying on TCL_INTERP_DESTROYED should instead check Tcl_InterpDeleted(). It is trivial to wrap Tcl_InterpDeleted() in Tcl.xs. I am not aware of any Tcl.pm users currently relying on Tcl::INTERP_DESTROYED, but if there are they can migrate to checking $interp->InterpDeleted().
    I am not aware of why this method is useful, particularly from Tcl.pm. I have not created a test for the new method, but I have verified that its return value is correct for a hacked version of Tcl_InterpDeleted().

@chrstphrchvz chrstphrchvz marked this pull request as draft April 22, 2022 06:30
@vadrer
Copy link
Collaborator

vadrer commented Apr 22, 2022

ok, nice. thanks!

@chrstphrchvz chrstphrchvz changed the title Tcl.xs: use pointers to const Tcl_ObjType Improve Tcl 8.7/9.0 readiness, const usage Apr 22, 2022
@chrstphrchvz chrstphrchvz force-pushed the patch-4 branch 2 times, most recently from ffd83b7 to 8a8b231 Compare April 22, 2022 08:48
@chrstphrchvz
Copy link
Contributor Author

chrstphrchvz commented Apr 23, 2022

I am trying to figure out how to properly deal with various types (int, wideInt, boolean, bytearray) no longer being registered in Tcl 9.0. This causes Tcl_GetObjType() to return NULL, which breaks SvFromTclObj() since it assumes cached type pointers are not NULL so that values with objPtr->typePtr == NULL fall through to the string case. Example: in disposal-subs-b.t, flush_afters() waits for after info to be empty, but SvFromTclObj() returns a garbage integer value instead of an empty list, and the test never exits. I wonder if SvFromTclObj() should more resemble Tkinter’s approach which checks for objPtr->typePtr == NULL first (see FromObj() in _tkinter.c; I am not aware of Tkinter having already prepared for Tcl 8.7/9.0, though).

I am also considering how to deal with Tcl TIP 484 which removes the wideInt type, meaning the int type is used for 64-bit values even on 32-bit platforms. I am not aware of a good approach for use in XS libraries to support 64-bit integer SVs. Currently I would be inclined to return a Tcl integer as a string unless it is within [IVMIN,UVMAX] (or maybe as an NV if within the range of double which can represent odd integers).

It sounds like upstream Tcl may be willing to help C API users migrate, as there are not very many which are still actively maintained (edit: see https://core.tcl-lang.org/tcl/info/3bb3bcf2da5b).

Documentation for Tcl_GetStringFromObj() recommends assigning
to `const char *`
Tcl_GetObjType() returns const as of Tcl 8.6,
and Tcl.xs never modifies these structs
Constify variables as needed

Convert existing `CONST`/`CONST84` usage to `const`
(This removes USE_NON_CONST compatibility, which should not be needed
since Tcl.pm has required Tcl 8.4 or later since 2004)

Needed for building with Tcl 9.0 (and Tcl 8.7 when TCL_NO_DEPRECATED
is defined) since tcl.h no longer defines CONST84 or recognizes
USE_NON_CONST.
Remove Tcl::INTERP_DESTROYED constant

Add method InterpDeleted()

Any existing checks relying on Tcl::INTERP_DESTROYED must migrate
to checking InterpDeleted() instead.
tcl.h normally does not define these macros in Tcl 9.0 (and Tcl 8.7
when TCL_NO_DEPRECATED is defined), and has allowed these macros
to already be defined since Tcl 8.4b2. Perl’s definitions for
these macros (for C compilers) are equivalent to those in tcl.h.
…i.e. various types merged or no longer registered in Tcl 8.7/9.0

Convert wideInt to IV/UV when possible

Convert string booleans (i.e. "true"/"false"/"yes"/"no"/"on"/"off")
to 0 or 1
Closes gisle#47
@chrstphrchvz
Copy link
Contributor Author

I am satisfied enough with the work in this pull request for it to be merged.

There is at least one remaining issue where Tcl.pm built to use stubs will not load with Tcl 9.0 (Tcl_InitStubs() cannot be called with a differing major version), but I am currently not interested in fixing this.

@chrstphrchvz chrstphrchvz marked this pull request as ready for review March 5, 2023 04:42
@vadrer vadrer merged commit 8747378 into gisle:master Mar 25, 2023
@chrstphrchvz chrstphrchvz deleted the patch-4 branch March 26, 2023 00: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.

2 participants