Re: [Tarantool-patches] [PATCH luajit v4 2/4] memprof: refactor location parsing

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Misha,

Thanks for the fixes! LGTM, except a tiny nit below.

On 04.11.21, Mikhail Shishatskiy wrote:
> Hi, Igor!
> Thank you for the review!
> 
> New commit message:
> ============================================================
> memprof: refactor location parsing
> 
> As it is hard to keep function `id_location` easily usable in the
> function `parse_location` with a growing number of fields in the
> <loc> table, the code is refactored to make it more understandable.
> 
> The API of <utils/symtab.lua> module changed with adding a public
> function `id(loc)` returning the same id-string, as `id_location`
> did. This function is needed to be used in other modules to
> "stringify" the location and use it as a key in the key/value
> storage.
> 
> Part of tarantool/tarantool#5814
> ============================================================
> 
> Diff:
> ============================================================
> diff --git a/tools/memprof/parse.lua b/tools/memprof/parse.lua
> index 34ff8aef..75f5cb39 100644
> --- a/tools/memprof/parse.lua
> +++ b/tools/memprof/parse.lua
> @@ -8,10 +8,10 @@ local bit = require "bit"
>   local band = bit.band
>   local lshift = bit.lshift
> 
> -local symtab = require "utils.symtab"
> -
>   local string_format = string.format
> 
> +local symtab = require "utils.symtab"
> +
>   local LJM_MAGIC = "ljm"
>   local LJM_CURRENT_VERSION = 1
> 
> @@ -66,12 +66,15 @@ local function parse_location(reader, asource)
>       addr = 0,
>       line = 0,
>     }
> -  if asource == ASOURCE_CFUNC then
> +  -- luacheck: ignore
> +  if asource == ASOURCE_INT then
> +    -- Do nothing.

I believe adding only luacheck suppression is enough here.

| ...
| if asource == ASOURCE_INT then -- luacheck: ignore
| else asource == ASOURCE_CFUNC then
| ...

> +  elseif asource == ASOURCE_CFUNC then
>       loc.addr = reader:read_uleb128()
>     elseif asource == ASOURCE_LFUNC then
>       loc.addr = reader:read_uleb128()
>       loc.line = reader:read_uleb128()
> -  elseif asource ~= ASOURCE_INT then
> +  else
>       error("Unknown asource "..asource)
>     end
>     return symtab.id(loc), loc
> ============================================================
> 
> Issue: https://github.com/tarantool/tarantool/issues/5814
> Branch: https://github.com/tarantool/luajit/tree/shishqa/gh-5814-group-allocations-on-trace-by-trace-number
> CI: https://github.com/tarantool/tarantool/tree/shishqa/gh-5814-group-allocations-on-trace-by-trace-number
> 

<snipped>

> --
> Best regards,
> Mikhail Shishatskiy

-- 
Best regards,
IM