NCI 'v' vs '' in function parameter signatures
Tim Bunce
Here, for example, is the code for 'fv' and 'f':
static void
pcf_f_v(Interp *interpreter, PMC *self)
{
typedef float (*func_t)(void);
func_t pointer;
struct call_state st;
float return_data;
Parrot_init_arg_nci(interpreter, &st, "v");
pointer = (func_t)D2FPTR(PMC_struct_val(self));
return_data = (float)(*pointer)();
set_nci_N(interpreter, &st, return_data);
}
static void
pcf_f(Interp *interpreter, PMC *self)
{
float (*pointer)(void);
float return_data;
struct call_state st;
pointer = (float (*)(void))D2FPTR(PMC_struct_val(self));
return_data = (float)(*pointer)();
set_nci_N(interpreter, &st, return_data);
}
The code is a little different but it's not clear (to me) if there's any
practical difference.
I ask because both 'fv' and 'f' are in src/call_list.txt
In fact there are several 'duplicated' signatures:
Ignored signature 'cv' on line 52 (previously seen on line 49)
Ignored signature 'dv' on line 58 (previously seen on line 54)
Ignored signature 'fv' on line 85 (previously seen on line 82)
Ignored signature 'iv' on line 150 (previously seen on line 87)
Ignored signature 'lv' on line 162 (previously seen on line 155)
Ignored signature 'pv' on line 187 (previously seen on line 170)
Ignored signature 'sv' on line 190 (previously seen on line 189)
Ignored signature 'tv' on line 202 (previously seen on line 192)
Ignored signature 'vv' on line 217 (previously seen on line 204)
Those warnings come from a version of tools/build/nativecall.pl I've
modified to 'normalize' the signatures to use 'v' and detect duplicates.
(As a side effect the nci.o file dropped from 354K to 347K.)
Also, what's the protocol for adding signatures to call_list.txt?
I've at least one I want to add ('p ttttitl' for mysql_real_connect)
and may have more soon. Should I just post a patch here?
Tim.
Leopold Toetsch
> What's the difference between 'v' and '' for NCI function parameters?
There isn't any, except the extra 'v' char.
> I ask because both 'fv' and 'f' are in src/call_list.txt
Yeah.
> In fact there are several 'duplicated' signatures:
[ ... ]
I'd say, we should drop all the '?v' variants. The extra 'v' doesn't
cover any information, it's just causing an init call to the argument
passing code.
> Those warnings come from a version of tools/build/nativecall.pl I've
> modified to 'normalize' the signatures to use 'v' and detect duplicates.
> (As a side effect the nci.o file dropped from 354K to 347K.)
Good. But as said, I'd prefer the shorter signature.
> Also, what's the protocol for adding signatures to call_list.txt?
> I've at least one I want to add ('p ttttitl' for mysql_real_connect)
> and may have more soon. Should I just post a patch here?
Yep.
> Tim.
Thanks for looking into this,
leo
Tim Bunce
I'll aim to work up a patch this week.
The runtime dlfunc code will need to be altered to normalize away the
trailing v so old code won't break. Should it warn about that?
Tim.
Chromatic
> I'd say, we should drop all the '?v' variants. The extra 'v' doesn't
> cover any information, it's just causing an init call to the argument
> passing code.
To avoid confusion, I suggest requiring that functions returning void always
use 'v' and disallowing the signature ''.
Allowing people to drop 'v' for the return in some cases seems like it will
bite them later when they try to write 'ff' for void foo ( float, float ) and
wonder why it doesn't work.
-- c
Leopold Toetsch
On Feb 14, 2006, at 18:29, Tim Bunce wrote:
> I'll aim to work up a patch this week.
Great, thanks.
>
> The runtime dlfunc code will need to be altered to normalize away the
> trailing v so old code won't break. Should it warn about that?
Yes, a warning please.
> Tim.
leo
Leopold Toetsch
On Feb 14, 2006, at 21:45, chromatic wrote:
>
> To avoid confusion, I suggest requiring that functions returning void
> always
> use 'v' and disallowing the signature ''.
Exactly my thoughts too.
leo
Tim Bunce
Here's the patch.
- removes 'v' argument entries from src/call_list.txt
- adds mysqlclient signatures to src/call_list.txt [*]
- tweaks docs/pdds/clip/pdd16_native_call.pod to match
- adds list of definition files used into generated nci.c
- adds sanity checking of return and argument sig chars
- adds compile time warning for deprecated 'v' argument
- adds optional duplicate signature warning (disabled)
- adds runtime warning for deprecated 'v' argument
Tim.
[*] I'm planning a followup patch that splits src/call_list.txt
into multiple files in a subdirectory. The mysqlclient defs would then
be in their own file. With the current arrangement no one can safely
remove a signature because there's no indication of what that signature
exists for.
The same ncidef file could then be used for both tools/build/nativecall.pl
and tools/util/ncidef2pasm.pl (which I also plan to work on).
Any objections or comments?
Tim Bunce
Tim.
> Index: src/call_list.txt
> ===================================================================
> --- src/call_list.txt (revision 11741)
> +++ src/call_list.txt (working copy)
> @@ -49,14 +49,12 @@
> c # t/pmc/nci.t
> c p
> c pi
> -c v
>
> d # t/pmc/nci.t
> d d
> d JOd # Parrot builtins
> I JOS
> S JOS # ParrotIO.readline
> -d v
> I JI # Parrot_is_char_*
> v JOSP # String.trans
> v JOS # String.reverse
> @@ -83,7 +81,6 @@
> f # t/pmc/nci.t
> f ff # t/pmc/nci.t
> f is
> -f v
>
> i
> i b # t/pmc/nci.t
> @@ -148,7 +145,6 @@
> i ssss
> i t
> i ti
> -i v
> i 4
> i 4i
> i 42p
> @@ -160,7 +156,6 @@
> l pi
> l pii
> l p33l
> -l v
> l 33l
>
> P Ji # Needed for parrot threads
> @@ -185,10 +180,8 @@
> p t
> p tpp
> p ttttttt
> -p v
>
> s # t/pmc/nci.t
> -s v
>
> t # t/pmc/nci.t
> t i
> @@ -200,7 +193,6 @@
> t t
> t tl4
> t t4
> -t v
>
> v
> v Jiiip # examples/japh/japh11.pasm
> @@ -215,7 +207,6 @@
> v piiii
> v pl
> v pp
> -v v
>
> # These are needed for parrotio.pmc
> i JP
> @@ -343,3 +334,77 @@
>
> # Make lua stop panic'ing.
> P JOI
> +
> +
> +# --- start mysqlclient library ---
> +# Created from mysql.h using the following manual method:
> +# Edited copy of mysql.h using vi by doing g/, *$/j (repeat) then g/\* *$/j (repeat)
> +# to get all functions on one line each.
> +# Extracted list of api func names from http://dev.mysql.com/doc/refman/4.1/en/c-api-functions.html
> +# and copied to a temporary file to clean up (mysql_api_names.txt)
> +# Stripped down to bare names and merged into one line separated by |
> +# then egrep -w `cat mysql_api_names.txt` mysql.h > mysql_api.ncidef
> +# then edit mysql_api.ncidef in vi: %s/^/ # /
> +# to create space for nci signatures and to use original definition as a # comment.
> +# This method isn't ideal, I'm just noting it here in case it helps others.
> +# Ideally the process should be automated - but there be many dragons along # that path.
> +#
> +# long long values (my_ulonglong) aren't handled by nci - spec'd as just long for now
> +#
> +# MYSQL_FIELD and MYSQL_RES are structs
> +# typedef char **MYSQL_ROW; /* return data as array of strings */
> +# typedef unsigned int MYSQL_FIELD_OFFSET; /* offset to current field */
> +# typedef MYSQL_ROWS *MYSQL_ROW_OFFSET; /* offset to current row */
> +#
> +l p #! my_ulonglong mysql_num_rows(MYSQL_RES *res)
> +i p # unsigned int mysql_num_fields(MYSQL_RES *res)
> +c p # my_bool mysql_eof(MYSQL_RES *res)
> +p pi # MYSQL_FIELD *mysql_fetch_field_direct(MYSQL_RES *res, unsigned int fieldnr)
> +p p # MYSQL_FIELD * mysql_fetch_fields(MYSQL_RES *res)
> +p p # MYSQL_ROW_OFFSET mysql_row_tell(MYSQL_RES *res)
> +i p # MYSQL_FIELD_OFFSET mysql_field_tell(MYSQL_RES *res)
> +i p # unsigned int mysql_field_count(MYSQL *mysql)
> +l p #! my_ulonglong mysql_affected_rows(MYSQL *mysql)
> +l p #! my_ulonglong mysql_insert_id(MYSQL *mysql)
> +i p # unsigned int mysql_errno(MYSQL *mysql)
> +t p # const char * mysql_error(MYSQL *mysql)
> +t p # const char * mysql_info(MYSQL *mysql)
> +l p # unsigned long mysql_thread_id(MYSQL *mysql)
> +t p # const char * mysql_character_set_name(MYSQL *mysql)
> +p p # MYSQL * mysql_init(MYSQL *mysql)
> +i pttttt # int mysql_ssl_set(MYSQL *mysql, const char *key, const char *cert, const char *ca, const char *capath, const char *cipher)
> +c pttt # my_bool mysql_change_user(MYSQL *mysql, const char *user, const char *passwd, const char *db)
> +p pttttiti # MYSQL * mysql_real_connect(MYSQL *mysql, const char *host, const char *user, const char *passwd, const char *db, unsigned int port, const char *unix_socket, unsigned int clientflag)
> +v p # void mysql_close(MYSQL *sock)
> +i pt # int mysql_select_db(MYSQL *mysql, const char *db)
> +i pt # int mysql_query(MYSQL *mysql, const char *q)
> +i ptl # int mysql_real_query(MYSQL *mysql, const char *q, unsigned long length)
> +i p # int mysql_shutdown(MYSQL *mysql)
> +i p # int mysql_dump_debug_info(MYSQL *mysql)
> +i pi # int mysql_refresh(MYSQL *mysql, unsigned int refresh_options)
> +i pl # int mysql_kill(MYSQL *mysql,unsigned long pid)
> +i p # int mysql_ping(MYSQL *mysql)
> +t p # const char * mysql_stat(MYSQL *mysql)
> +t p # const char * mysql_get_server_info(MYSQL *mysql)
> +t p # const char * mysql_get_client_info(void)
> +l # unsigned long mysql_get_client_version(void)
> +t p # const char * mysql_get_host_info(MYSQL *mysql)
> +t p # unsigned int mysql_get_proto_info(MYSQL *mysql)
> +p pt # MYSQL_RES * mysql_list_dbs(MYSQL *mysql,const char *wild)
> +p pt # MYSQL_RES * mysql_list_tables(MYSQL *mysql,const char *wild)
> +p ptt # MYSQL_RES * mysql_list_fields(MYSQL *mysql, const char *table, const char *wild)
> +p p # MYSQL_RES * mysql_list_processes(MYSQL *mysql)
> +p p # MYSQL_RES * mysql_store_result(MYSQL *mysql)
> +p p # MYSQL_RES * mysql_use_result(MYSQL *mysql)
> +i pit # int mysql_options(MYSQL *mysql,enum mysql_option option, const char *arg)
> +v p # void mysql_free_result(MYSQL_RES *result)
> +v pl # void mysql_data_seek(MYSQL_RES *result, my_ulonglong offset)
> +p pp # MYSQL_ROW_OFFSET mysql_row_seek(MYSQL_RES *result, MYSQL_ROW_OFFSET offset)
> +i pi # MYSQL_FIELD_OFFSET mysql_field_seek(MYSQL_RES *result, MYSQL_FIELD_OFFSET offset)
> +p p # MYSQL_ROW mysql_fetch_row(MYSQL_RES *result)
> +l p # unsigned long * mysql_fetch_lengths(MYSQL_RES *result)
> +p p # MYSQL_FIELD * mysql_fetch_field(MYSQL_RES *result)
> +l ttl # unsigned long mysql_escape_string(char *to,const char *from, unsigned long from_length)
> +l pttl # unsigned long mysql_real_escape_string(MYSQL *mysql, char *to,const char *from, unsigned long length)
> +v t # void mysql_debug(const char *debug)
> +# --- end mysqlclient library ---
> Index: docs/pdds/clip/pdd16_native_call.pod
> ===================================================================
> --- docs/pdds/clip/pdd16_native_call.pod (revision 11741)
> +++ docs/pdds/clip/pdd16_native_call.pod (working copy)
> @@ -51,10 +51,12 @@
>
> =item v
>
> -Void. As a return type indicates that there I<is> no return type. As a
> -parameter indicates that there are no parameters. Can't be mixed with other
> -parameter types.
> +Void. As a return type it indicates that there I<is> no return type.
>
> +As a parameter it indicates that there are no parameters (this use is now
> +deprecated - use an empty parameter string to indicate that there are no
> +parameters). Can't be mixed with other parameter types.
> +
> =item c
>
> Char. This is an integer type, taken from (or put into) an I register. NOTE: it
> Index: tools/build/nativecall.pl
> ===================================================================
> --- tools/build/nativecall.pl (revision 11741)
> +++ tools/build/nativecall.pl (working copy)
> @@ -31,11 +31,12 @@
> use strict;
> use warnings;
>
> +my $opt_warndups = 0;
>
> # This file will eventually be compiled
> open NCI, ">", "src/nci.c" or die "Can't open nci.c!";
>
> -print_head();
> +print_head(\@ARGV);
>
> my %ret_type =
> ( p => "void *",
> @@ -187,17 +188,33 @@
> s/\s*$//;
> next unless $_;
>
> + my ($ret, $args) = split /\s+/, $_;
> +
> + $args = '' if not defined $args;
> + $args =~ s/^v$//
> + and warn "Removed deprecated 'v' argument signature on line $. of $ARGV";
> +
> + die "Invalid return signature char '$ret' on line $. of $ARGV"
> + unless exists $ret_assign{$ret};
> +
> + if (($seen{"$ret$args"} ||= $.) != $.) {
> + warn sprintf "Ignored signature '%s' on line %d (previously seen on line %d) of $ARGV",
> + "$ret$args", $., $seen{"$ret$args"}
> + if $opt_warndups;
> + next;
> + }
> +
> my @extra_preamble;
> my @extra_postamble;
> my @temps;
> - my ($ret, $args) = split /\s+/, $_;
> - ## next if $seen{"$ret$;$args"}++;
> my @arg;
> my $reg_num = 0;
> my $sig = '';
>
> if (defined $args and not $args =~ m/^\s*$/ ) {
> foreach (split //, $args) {
> + die "Invalid argument signature char '$_' on line $. of $ARGV"
> + unless exists $sig_char{$_};
> push @arg, make_arg($_, $reg_num++, \$temp_cnt, \@temps,
> \@extra_preamble, \@extra_postamble);
> $sig .= $sig_char{$_};
> @@ -220,11 +237,13 @@
>
>
> sub print_head {
> + my ($definitions) = @_;
> print NCI << "HEAD";
> /*
> * !!!!!!! DO NOT EDIT THIS FILE !!!!!!!
> *
> - * This file is generated automatically by tools/build/nativecall.pl.
> + * This file is generated automatically by tools/build/nativecall.pl
> + * from definitions in @$definitions
> *
> * Any changes made here will be lost!
> *
> @@ -490,7 +509,7 @@
> qq{$ret_type_decl return_data;} :
> q{};
>
> - if (defined $params) {
> + if (length $params) {
> my $proto = join ', ', map { $proto_type_ref->{$_} } split( '', $params );
> # This is an after-the-fact hack: real fix would be in make_arg
> # or somewhere at that level. The main point being that one cannot
> @@ -542,7 +561,7 @@
> $call_state = '' if 'v' eq $return;
> print NCI << "HEADER";
> static void
> -pcf_${return}(Interp *interpreter, PMC *self)
> +pcf_${return}_(Interp *interpreter, PMC *self)
> {
> $ret_type (*pointer)(void);
> $return_data
> @@ -590,6 +609,7 @@
> PMC *b;
> PMC *iglobals;
> PMC *temp_pmc;
> + UINTVAL signature_len;
>
> void *result = NULL;
> Hash *known_frames = NULL;
> @@ -610,7 +630,14 @@
> /* And in here is the platform-independent way. Which is to say
> "here there be hacks" */
> UNUSED(pmc_nci);
> - if (0 == string_length(interpreter, signature)) return F2DPTR(pcf_v_v);
> + signature_len = string_length(interpreter, signature);
> + if (0 == signature_len) return F2DPTR(pcf_v_);
> + /* remove deprecated void argument 'v' character */
> + if (2 == signature_len && 'v' == string_index(interpreter, signature, 1)) {
> + Parrot_warn(interpreter, PARROT_WARNINGS_ALL_FLAG, "function signature argument character 'v' ignored");
> + signature = string_chopn(interpreter, signature, 1, 1);
> + signature_len = string_length(interpreter, signature);
> + }
>
> iglobals = interpreter->iglobals;
>
Chromatic
> Any news on this? Is it okay? Should I send it via parrotbug?
It looked good to me. I say check it in and see what the smokes do.
-- c