[Tarantool-patches] [PATCH luajit][v2] Always close profiler output file.

[Sergey Bronnikov]

Hi, Sergey!

The second testcase has been added and

changes force-pushed to the branch.

Sergey

On 04.03.2025 18:40, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the fixes, LGTM.
>
> On 27.02.25, Sergey Bronnikov wrote:
>> Hi, Sergey,
>>
>> thanks for review!
>>
>> Changes applied and force-pushed.
>>
>> Sergey
>>
>> On 26.02.2025 14:58, Sergey Kaplun wrote:
> <snipped>
>
>>>> What is a rationale for this?
>>> As I said before, this is an additional possible demonstration of the
>>> bug. I see nothing bad to add it as well, since it simulates a little
>>> bit different workload -- flushing the output before we finish the
>>> process or if we unload the module. I am not insisting on it, though.
>> How often users unload profiler's module? For me, this case looks artificial
> The following module [1] is used to reload all Lua modules (including
> `jit.p`). It was used for any Lua package update or code modification.
>
> [1]:https://github.com/moonlibs/package-reload
>
> Also, it is not only about unloading the module but also about finishing
> the process too (as you first discovered). This looks like a real use
> case. This variant of the test is just simpler than creating a child
> process and checking its result files.
>
>> and the final goal for us is not a demonstration, but covering a code
>> touched by the patch,
>>
>> it is done by proposed test.
>>
>> (I'm really confused that on review I learn more and more new
>> requirements to the patches
> It is not a requirement, just a suggestion. See the last line of my
> comment. I'm OK with reproducing the issue only since it is not the
> vital part of the code.
>
>> like adding all available testcases that cover a patch or that code
>> should follow C89 or
>>
>> that using etc.)
>>
>> The bug fixed by the patch is quite minor, it will not break production
>> and will not kill humans etc.
>>
>> I believe a single testcase and time that we both spend on doing
>> backport, test and two iterations
>>
>> of review is more than enough for this patch. Moreover, jit.p is not
>> used by Tarantool users,
>>
>> we provide our own profilers to them.
> So, just ignore it then.
>
>>
>>>> This is a helper script for LuaJIT profiler and according to our
>>>> backporting rules
>>>>
>>>> test must cover a backported patch. This rule is fulfilled by my test.
>>> <snipped>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20250305/caabb16f/attachment.htm>