Re: [Tarantool-patches] [PATCH luajit 8/8][v3] memprof: set default path to profiling output file

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergey!
See my answer below.

On 24.02.25, Sergey Bronnikov wrote:
> Hi, Sergey,
> 
> thanks for review!

<snipped>

> >> ---
> >> +
> >> +  local default_output_file = 'memprof.bin'
> >> +  os.remove(default_output_file)
> >> +
> >> +  local _, _ = misc.memprof.start()
> > Minor: Do we want to check that the profiler starts successfully?
> > I suppose we should, since this is the expected behaviour for this
> > feature. In case the profiler is not started (old behaviour) we would
> > get an error from the branch below: profiler not running. This isn't a
> > verbose definition of what goes wrong.
> 
> I don't get why we should check that profiler is started in a test for 
> default output file.

If the `memprof.start()` call will fail in the test (for any reason),
the next call of `memprof.stop()` will return `false, "profiler is not
running"`. So, when debugging the test, we have no clue about the reason
for the not-started profiler, which is not very convenient.

> > I suppose we may use `goto` here like the following:
> >
> > |   local res, err = misc.memprof.start()
> > |   -- Should start successfully.
> > |   if not res then
> > |     goto err_handling
> > |   end
> > |
> > |   res, err = misc.memprof.stop()
> > |
> > | ::err_handling::
> > |   -- Want to cleanup carefully if something went wrong.
> > |   if not res then
> >
> >> +
> >> +  local res, err = misc.memprof.stop()
> >> +
> >> +  -- Want to cleanup carefully if something went wrong.
> >> +  if not res then
> >> +    os.remove(default_output_file)
> >> +    error(err)
> >> +  end

<snipped>

-- 
Best regards,
Sergey Kaplun