Re: Teng Long
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Mar 29, 2022, 08:54 UTC
- Message-ID
- <220329.86pmm52kzf.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <20220329073851.80702-1-dyroneteng@gmail.com>
On Tue, Mar 29 2022, Teng Long wrote:
Show 5 quoted lines
>> Was there an on-list v0 (RFC?) or is this a range-diff against nothing? >> Best not to include it until a v2 then. > > My careless, there's no RFC, actually this patch should begin with v0 and > without range-diff.
No worries.
Show 13 quoted lines
>> Sometimes it's better to split up patches, but I think these 3x should
>> really be squashed together. We make incremental progress to nowhere in
>> 1/3 and 2/3, and it all comes together in 3/3. The 1-2/3 are trivial
>> enough that we can squash them in.
>
> Sure.
>
>> + trace2_data_string("midx", r, "core.multipackIndex",
>> + r->settings.core_multi_pack_index ? "true" : "false");
>>
>> Weird indentation here.
>
> Will fix.*Nod*
Show 6 quoted lines
>> I.e. surely you just want to create a region, and if you care enough to >> log failure shouldn't we log that in open_midx_bitmap_1() if we care, > > Actually, I just want to use "trace2_data_string()" at first because which informations I > want to append is not so many, or does "create a region" a preferred principle for using > TRACE2 APIs?
I think a begin/end region gives you everything you want here and more, i.e.:
* We'll get start/end times on this (potentially expensive?) operation automatically.
* You just log a "failed", but as the RFC-on-top shows we can just log the specific reason instead, either via error(), warning() or perhaps even trace2_data_string(). because it's within a region it'll be clear from the data what failed.
B.t.w. I noticed after sending that that this ad-hoc patch fails the tests, it passed the one bitmap test I hacked it up against.
It was just there to make the point/demo that it looked easier to do this one function call above where you were adding this.
>> and perhaps error() there instead of silently returning -1? > > I think so, after I checked function "error_builtin()" and there is a "trace2_cmd_error_va()" > usage in it which is already support to print some debug infos using TRACE2 envs.
Exactly.