[DTrace-devel] [PATCH v4 19/20] Implement a framework for D subroutines incl. strlen() and speculation() implementation

[Eugene Loh]

Reviewed-by: Eugene Loh <eugene.loh at oracle.com>

with a few nits.  E.g., do we have a length limit on commit subject 
lines?  It might make sense to omit the trailing word "implementation" 
or even to ditch mention of the specific subroutines given that, e.g., 
speculation isn't even really done yet.  And...

On 6/3/21 11:20 AM, Kris Van Hees wrote:
> This patch provides the skeleton for the implementation of subroutines
> in D.  It is equivalent to the implementation of D actions in the code
> generator.
>
> The strlen() subroutine is provided as a first example of how to
> imeplement subroutines.  The speculation() subroutine is provided as

imeplement => implement

> an example of a dummy implementation.  It always simply returns 0, but
> this allows various tests for the compiler to be exercised.
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> +static void
> +dt_cg_call_subr(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> +{
> +	dt_ident_t	*idp = dnp->dn_ident;
> +	dt_cg_subr_f	*fun;
> +
> +	if (idp->di_kind != DT_IDENT_FUNC)
> +		dnerror(dnp, D_CG_EXPR, "%s %s( ) may not be called from a D "
> +					"expression (D program context "
> +					"required)\n",
> +			dt_idkind_name(idp->di_kind), idp->di_name);

It's nice to reduce line breaks in error message strings so that someone 
who gets a message has an easier time grepping for it.  So, if we're 
going >80 chars on the subject line, how about going >80 chars in a 
useful place like this?  Or, at least improve the breaks in the string.  
Say:
         if (idp->di_kind != DT_IDENT_FUNC)
                 dnerror(dnp, D_CG_EXPR,
                         "%s %s( ) may not be called from a D expression "
                         "(D program context required)\n",
                         dt_idkind_name(idp->di_kind), idp->di_name);

> +
> +	assert(idp->di_id > 0 && idp->di_id <= DIF_SUBR_MAX);

 > should probably be >=

> +
> +	fun = _dt_cg_subr[idp->di_id];
> +	if (fun == NULL)
> +		dnerror(dnp, D_FUNC_UNDEF, "unimplemented subroutine: %s\n",
> +			idp->di_name);
> +	fun(dnp, dlp, drp);
> +}