Re: [PATCH broken] PR10257: Add support for sprint[ln](@hist_*).

[Przemysław Pawełczyk <przemyslaw at pawelczyk dot it>]

On Mon, Oct 12, 2009 at 22:53, Josh Stone <jistone@redhat.com> wrote:
> First, I'm skeptical of how useful sprint for histograms will be -- it seems
> like it will quite easily exceed MAXSTRINGLEN. ?I'm pretty sure this is why
> we haven't already enabled it. ?Did you have some specific use in mind?

Yes, I have, but it's tightly connected with PR10690 - statistics on
demand via procfs. Sometimes running stap scripts in daemon mode is
just more convenient.

> On 10/10/2009 02:51 PM, Przemyslaw Pawelczyk wrote:
>>
>> Currently this patch doesn't work, because tmpvar added in sprint case
>> in c_unparser::visit_print_format() doesn't increase number of __tmp
>> variables available in struct probe_XYZ_locals that is generated by
>> c_unparser::emit_common_header(). I certainly missed sth, but what?
>> Any help will be appreciated. Reviews are also welcomed.
>
> There is also c_tmpcounter which needs to mirror the tmp allocations. This
> is a sort of pre-visitor which creates the tmps in the context struct.

I see...

>
>> +static void _stp_stat_print_histogram_buf(char *buf, Hist st, stat *sd)
>
> So much duplication with _stp_stat_print_histogram is not ok -- can you make
> one a function of the other? ?It may help you to know that these are
> equivalent:
>
> ?_stp_printf(fmt, ...args...);
> ?_stp_snprintf(NULL, 0, fmt, ...args...);

I'm an idiot, it's obvious. Still nobuf version will be then slightly
bloated with unnecessary code, but I agree that DRY-ness and
maintainability are more important here, so this trade-off is fully
acceptable. I'll send new patch later.

> Josh

Thank you for your help and review.

-- 
Przemysław Pawełczyk