ronantakizawa/codereview-bench huggingface dataset:
Repository: neovim/neovim
File: src/gen/gen_vimdoc.lua (Lua)

Code before:
    files = {
      'runtime/lua/vim/health.lua',
    },
    section_order = {
      'health.lua',
    },
    section_fmt = function(_name)
      return 'Checkhealth'
    end,
    helptag_fmt = function()
      return { 'vim.health', 'health' }
    end,
  },
  pack = {
    filename = 'pack.txt',
    files = { 'runtime/lua/vim/pack.lua' },
    section_order = { 'pack.lua' },
    section_fmt = function(_name)
      return 'Plugin manager'
    end,
    helptag_fmt = function()
      return { 'vim.pack' }
    end,
  },
  plugins = {
    filename = 'runtime-plugins.txt',
    section_order = {
      'tohtml.lua',
    },
    files = {
      'runtime/lua/tohtml.lua',
    },
    section_fmt = function(name)
      return 'Builtin plugin: ' .. name:lower()
    end,
    helptag_fmt = function(name)
      return name:lower()
    end,
  },
}

--- @param ty string
--- @param generics table<string,string>
--- @return string
local function replace_generics(ty, generics)
  if ty:sub(-2) == '[]' then
    local ty0 = ty:sub(1, -3)
    if generics[ty0] then
      return generics[ty0] .. '[]'
    end
  elseif ty:sub(-1) == '?' then

Review comment (suggestion):
"runtime" is mostly redundant

```suggestion
    filename = 'plugins.txt',
```

Code after:
    files = {
      'runtime/lua/vim/health.lua',
    },
    section_order = {
      'health.lua',
    },
    section_fmt = function(_name)
      return 'Checkhealth'
    end,
    helptag_fmt = function()
      return { 'vim.health', 'health' }
    end,
  },
  pack = {
    filename = 'pack.txt',
    files = { 'runtime/lua/vim/pack.lua' },
    section_order = { 'pack.lua' },
    section_fmt = function(_name)
      return 'Plugin manager'
    end,
    helptag_fmt = function()
      return { 'vim.pack' }
    end,
  },
  plugins = {
    filename = 'plugins.txt',
    section_order = {
      'tohtml.lua',
    },
    files = {
      'runtime/lua/tohtml.lua',
    },
    section_fmt = function(name)
      return 'Builtin plugin: ' .. name:lower()
    end,
    helptag_fmt = function(name)
      return name:lower()
    end,
  },
}

--- @param ty string
--- @param generics table<string,string>
--- @return string
local function replace_generics(ty, generics)
  if ty:sub(-2) == '[]' then
    local ty0 = ty:sub(1, -3)
    if generics[ty0] then
      return generics[ty0] .. '[]'
    end
  elseif ty:sub(-1) == '?' then
Repository: neovim/neovim
File: src/nvim/statusline.c (C)

Code before:
    }
    buf = click_recs[i].start;
    cur_click_def = click_recs[i].def;
    if (!tabline && !(cur_click_def.type == kStlClickDisabled
                      || cur_click_def.type == kStlClickFuncRun)) {
      // window bar and status line only support click functions
      cur_click_def.type = kStlClickDisabled;
    }
  }
  if (col < width) {
    while (col < width) {
      click_defs[col++] = cur_click_def;
    }
  } else {
    xfree(cur_click_def.func);
  }
}

/// Redraw the status line, window bar or ruler of window "wp".
/// When "wp" is NULL redraw the tab pages line from 'tabline'.
/// When "external" is true emit a UI-event instead (only for ruler currently).
static void win_redr_custom(win_T *wp, bool draw_winbar, bool draw_ruler, bool external)
{
  static bool entered = false;
  int col = 0;
  int attr, group, row, maxwidth;
  schar_T fillchar;
  char buf[MAXPATHL];
  char transbuf[MAXPATHL];
  char *stl;
  OptIndex opt_idx = kOptInvalid;
  int opt_scope = 0;
  stl_hlrec_t *hltab;
  StlClickRecord *tabtab;
  bool is_stl_global = global_stl_height() > 0;

  ScreenGrid *grid = &default_grid;

  // There is a tiny chance that this gets called recursively: When
  // redrawing a status line triggers redrawing the ruler or tabline.
  // Avoid trouble by not allowing recursion.
  if (entered) {
    return;
  }
  entered = true;

  // setup environment for the task at hand
  if (wp == NULL) {
    // Use 'tabline'.  Always at the first line of the screen.
    stl = p_tal;
    row = 0;

Review comment (suggestion):
```suggestion
  hlf_T group;
  int attr, row, maxwidth;
```

Code after:
    }
    buf = click_recs[i].start;
    cur_click_def = click_recs[i].def;
    if (!tabline && !(cur_click_def.type == kStlClickDisabled
                      || cur_click_def.type == kStlClickFuncRun)) {
      // window bar and status line only support click functions
      cur_click_def.type = kStlClickDisabled;
    }
  }
  if (col < width) {
    while (col < width) {
      click_defs[col++] = cur_click_def;
    }
  } else {
    xfree(cur_click_def.func);
  }
}

/// Redraw the status line, window bar, ruler or tabline.
/// @param wp  target window, NULL for 'tabline'
/// @param draw_winbar  redraw 'winbar'
/// @param draw_ruler  redraw 'rulerformat'
/// @param ui_event  emit UI-event instead of drawing
static void win_redr_custom(win_T *wp, bool draw_winbar, bool draw_ruler, bool ui_event)
{
  static bool entered = false;
  int col = 0;
  int attr, row, maxwidth;
  hlf_T group;
  schar_T fillchar;
  char buf[MAXPATHL];
  char transbuf[MAXPATHL];
  char *stl;
  OptIndex opt_idx = kOptInvalid;
  int opt_scope = 0;
  stl_hlrec_t *hltab;
  StlClickRecord *tabtab;
  bool is_stl_global = global_stl_height() > 0;

  ScreenGrid *grid = &default_grid;

  // There is a tiny chance that this gets called recursively: When
  // redrawing a status line triggers redrawing the ruler or tabline.
  // Avoid trouble by not allowing recursion.
  if (entered) {
    return;
  }
  entered = true;

  // setup environment for the task at hand
  if (wp == NULL) {
Repository: neovim/neovim
File: src/nvim/statusline.c (C)

Code before:
    did_ruler_col = 1;
  } else {
    if (did_show_ext_ruler) {
      ui_call_msg_ruler((Array)ARRAY_DICT_INIT);
      did_show_ext_ruler = false;
    }
    // Truncate at window boundary.
    for (n1 = 0, n2 = 0; buffer[n1] != NUL; n1 += utfc_ptr2len(buffer + n1)) {
      n2 += utf_ptr2cells(buffer + n1);
      if (this_ru_col + n2 > width) {
        bufferlen = n1;
        buffer[bufferlen] = NUL;
        break;
      }
    }

    grid_line_start(&msg_grid_adj, Rows - 1);
    did_ruler_col = off + this_ru_col;
    int w = grid_line_puts(did_ruler_col, buffer, -1, attr);
    grid_line_fill(did_ruler_col + w, off + width, fillchar, attr);
    grid_line_flush();
  }
}

/// Get the character to use in a status line.  Get its attributes in "*attr".
schar_T fillchar_status(int *group, win_T *wp)
{
  if (wp == curwin) {
    *group = HLF_S;
    return wp->w_p_fcs_chars.stl;
  } else {
    *group = HLF_SNC;
    return wp->w_p_fcs_chars.stlnc;
  }
}

/// Redraw the status line according to 'statusline' and take care of any
/// errors encountered.
void redraw_custom_statusline(win_T *wp)
{
  static bool entered = false;

  // When called recursively return.  This can happen when the statusline
  // contains an expression that triggers a redraw.
  if (entered) {
    return;
  }
  entered = true;

  win_redr_custom(wp, false, false, false);
  entered = false;

Review comment (suggestion):
```suggestion
schar_T fillchar_status(hlf_T *group, win_T *wp)
```

Code after:
    ADD_C(chunk, CSTR_AS_OBJ(buffer));
    ADD_C(chunk, INTEGER_OBJ(HLF_MSG));
    assert(attr == HL_ATTR(HLF_MSG));
    ADD_C(content, ARRAY_OBJ(chunk));
    ui_call_msg_ruler(content);
    did_show_ext_ruler = true;
    did_ruler_col = 1;
  } else {
    if (did_show_ext_ruler) {
      ui_call_msg_ruler((Array)ARRAY_DICT_INIT);
      did_show_ext_ruler = false;
    }
    // Truncate at window boundary.
    for (n1 = 0, n2 = 0; buffer[n1] != NUL; n1 += utfc_ptr2len(buffer + n1)) {
      n2 += utf_ptr2cells(buffer + n1);
      if (this_ru_col + n2 > width) {
        bufferlen = n1;
        buffer[bufferlen] = NUL;
        break;
      }
    }

    grid_line_start(&msg_grid_adj, Rows - 1);
    did_ruler_col = off + this_ru_col;
    int w = grid_line_puts(did_ruler_col, buffer, -1, attr);
    grid_line_fill(did_ruler_col + w, off + width, fillchar, attr);
    grid_line_flush();
  }
}

/// Get the character to use in a status line.  Get its attributes in "*attr".
schar_T fillchar_status(hlf_T *group, win_T *wp)
{
  if (wp == curwin) {
    *group = HLF_S;
    return wp->w_p_fcs_chars.stl;
  } else {
    *group = HLF_SNC;
    return wp->w_p_fcs_chars.stlnc;
  }
}

/// Redraw the status line according to 'statusline' and take care of any
/// errors encountered.
void redraw_custom_statusline(win_T *wp)
{
  static bool entered = false;

  // When called recursively return.  This can happen when the statusline
  // contains an expression that triggers a redraw.
  if (entered) {
Repository: neovim/neovim
File: runtime/lua/vim/lsp/rpc.lua (Lua)

Code before:

--- @async
local function request_parser_loop()
  local buf = strbuffer.new()
  while true do
    local msg = buf:tostring()
    local header_end = msg:find('\r\n\r\n', 1, true)
    if header_end then
      local header = buf:get(header_end + 1)
      buf:skip(2) -- skip past header boundary
      local content_length = get_content_length(header)
      while strbuffer.len(buf) < content_length do
        buf:put(coroutine.yield())
      end
      local body = buf:get(content_length)
      buf:put(coroutine.yield(body))
    else
      buf:put(coroutine.yield())
    end
  end
end

--- @private
--- @param handle_body fun(body: string)
--- @param on_exit? fun()
--- @param on_error? fun(errkind: vim.lsp.rpc.ClientErrors, err: any)
function M.create_read_loop(handle_body, on_exit, on_error)
  on_exit = on_exit or function() end
  on_error = on_error or function() end
  local co = coroutine.create(request_parser_loop)
  coroutine.resume(co)
  return function(err, chunk)
    if err then
      on_error(M.client_errors.READ_ERROR, err)
      return
    end

    if not chunk then
      on_exit()
      return
    end

    if coroutine.status(co) == 'dead' then
      return
    end

    while true do
      local ok, res = coroutine.resume(co, chunk)
      if not ok then
        on_error(M.client_errors.INVALID_SERVER_MESSAGE, res)
        break

Review comment (suggestion):
Technically this is a breaking change. But it's marked as `@private` so not going to worry. But I would probably add `errkind` as the 2nd arg just to avoid accidents.

Code after:

--- @async
local function request_parser_loop()
  local buf = strbuffer.new()
  while true do
    local msg = buf:tostring()
    local header_end = msg:find('\r\n\r\n', 1, true)
    if header_end then
      local header = buf:get(header_end + 1)
      buf:skip(2) -- skip past header boundary
      local content_length = get_content_length(header)
      while strbuffer.len(buf) < content_length do
        buf:put(coroutine.yield())
      end
      local body = buf:get(content_length)
      buf:put(coroutine.yield(body))
    else
      buf:put(coroutine.yield())
    end
  end
end

--- @private
--- @param handle_body fun(body: string)
--- @param on_exit? fun()
--- @param on_error? fun(err: any, errkind: vim.lsp.rpc.ClientErrors)
function M.create_read_loop(handle_body, on_exit, on_error)
  on_exit = on_exit or function() end
  on_error = on_error or function() end
  local co = coroutine.create(request_parser_loop)
  coroutine.resume(co)
  return function(err, chunk)
    if err then
      on_error(err, M.client_errors.READ_ERROR)
      return
    end

    if not chunk then
      on_exit()
      return
    end

    if coroutine.status(co) == 'dead' then
      return
    end

    while true do
      local ok, res = coroutine.resume(co, chunk)
      if not ok then
        on_error(res, M.client_errors.INVALID_SERVER_MESSAGE)
        break
Repository: neovim/neovim
File: runtime/lua/vim/_meta/json.lua (Lua)

Code before:
---                                   to Lua `nil` instead of |vim.NIL|.
---                                 - array: (boolean) When true, converts `null` in JSON arrays
---                                   to Lua `nil` instead of |vim.NIL|.
---@return any
function vim.json.decode(str, opts) end

--- Encodes (or "packs") a Lua object to stringified JSON.
---
--- Example: Implement a basic 'formatexpr' for JSON, so |gq| with a motion formats JSON in
--- a buffer. (The motion must operate on a valid JSON object.)
---
--- ```lua
--- function _G.fmt_json()
---   local indent = vim.bo.expandtab and (' '):rep(vim.o.shiftwidth) or '\t'
---   local lines = vim.api.nvim_buf_get_lines(0, vim.v.lnum - 1, vim.v.lnum + vim.v.count - 1, true)
---   local o = vim.json.decode(table.concat(lines, '\n'))
---   local stringified = vim.json.encode(o, { indent = indent, sort_keys = true })
---   lines = vim.split(stringified, '\n')
---   vim.api.nvim_buf_set_lines(0, vim.v.lnum - 1, vim.v.count, true, lines)
--- end
--- vim.o.formatexpr = 'v:lua.fmt_json()'
--- ```
---
---@param obj any
---@param opts? vim.json.encode.Opts
---@return string
function vim.json.encode(obj, opts) end


Review comment (suggestion):
I find it a bit inconsistent that we have `vim.json.encode.Opts` but not `vim.json.decode.Opts`. Are there any conditions that should be met to factor out the opts table?
I've also noticed that in other parts of the Lua API.

Code after:
--- ```
---
---@param str string Stringified JSON data.
---@param opts? vim.json.decode.Opts
---@return any
function vim.json.decode(str, opts) end

--- Encodes (or "packs") a Lua object to stringified JSON.
---
--- Example: Implement a basic 'formatexpr' for JSON, so |gq| with a motion formats JSON in
--- a buffer. (The motion must operate on a valid JSON object.)
---
--- ```lua
--- function _G.fmt_json()
---   local indent = vim.bo.expandtab and (' '):rep(vim.o.shiftwidth) or '\t'
---   local lines = vim.api.nvim_buf_get_lines(0, vim.v.lnum - 1, vim.v.lnum + vim.v.count - 1, true)
---   local o = vim.json.decode(table.concat(lines, '\n'))
---   local stringified = vim.json.encode(o, { indent = indent, sort_keys = true })
---   lines = vim.split(stringified, '\n')
---   vim.api.nvim_buf_set_lines(0, vim.v.lnum - 1, vim.v.count, true, lines)
--- end
--- vim.o.formatexpr = 'v:lua.fmt_json()'
--- ```
---
---@param obj any
---@param opts? vim.json.encode.Opts
---@return string
function vim.json.encode(obj, opts) end
Repository: neovim/neovim
File: test/functional/lua/ui_spec.lua (Lua)

Code before:
          stdout = 'arg1=arg1;arg2=https://example.com;',
        },
        exec_lua(function(cmd_)
          local cmd, err = vim.ui.open('https://example.com', { cmd = cmd_ })
          assert(cmd and not err)
          return cmd:wait()
        end, { n.testprg('printargs-test'), 'arg1' })
      )
    end)

    describe('tags in help files', function()
      local link_ns

      n.command('helptags $VIMRUNTIME/doc')

      before_each(function()
        n.clear({ args = { '-u', 'NORC' } })
        n.command('help nvim.txt')
        link_ns = n.api.nvim_create_namespace('nvim.help.urls')
      end)

      it('creates extmarks', function()
        t.neq(0, #n.api.nvim_buf_get_extmarks(0, link_ns, 0, -1, {}))
      end)

      it('points to https://neovim.io/doc/', function()
        local link =
          n.api.nvim_buf_get_extmarks(0, link_ns, 0, -1, { limit = 1, details = true })[1]

        local url = link[4].url
        assert(url)

        eq(true, vim.startswith(url, 'https://neovim.io/doc'))

        local param = url:match('%?tag=(.*)')
        local tag =
          n.api.nvim_buf_get_text(0, link[2], link[3], link[4].end_row, link[4].end_col, {})[1]
        eq(vim.uri_encode(tag), param)
      end)
    end)
  end)
end)


Review comment (suggestion):

can be a single it()

```suggestion
    it('gx on a help tag opens URL', function()
      n.command('helptags $VIMRUNTIME/doc')
      n.command('help nvim.txt')
      local link_ns = n.api.nvim_create_namespace('nvim.help.urls')

      -- creates extmarks
      t.neq(0, #n.api.nvim_buf_get_extmarks(0, link_ns, 0, -1, {}))
```

Code after:
          stdout = 'arg1=arg1;arg2=https://example.com;',
        },
        exec_lua(function(cmd_)
          local cmd, err = vim.ui.open('https://example.com', { cmd = cmd_ })
          assert(cmd and not err)
          return cmd:wait()
        end, { n.testprg('printargs-test'), 'arg1' })
      )
    end)

    it('gx on a help tag opens URL', function()
      n.command('helptags $VIMRUNTIME/doc')
      n.command('help nvim.txt')

      local link_ns = n.api.nvim_create_namespace('nvim.help.urls')
      local tag = n.api.nvim_buf_get_extmarks(0, link_ns, 0, -1, {
        limit = 1,
        details = true,
      })[1]

      local url = tag[4].url
      assert(url)

      --- points to the neovim.io site
      eq(true, vim.startswith(url, 'https://neovim.io/doc'))

      -- tag is URI encoded
      local param = url:match('%?tag=(.*)')
      local tagname =
        n.api.nvim_buf_get_text(0, tag[2], tag[3], tag[4].end_row, tag[4].end_col, {})[1]
      eq(vim.uri_encode(tagname), param)
    end)
  end)
end)
Repository: neovim/neovim
File: src/nvim/autocmd_defs.h (C/C++)

Code before:

/// Struct used to keep status while executing autocommands for an event.
typedef struct AutoPatCmd_S AutoPatCmd;
struct AutoPatCmd_S {
  AutoPat *lastpat;         ///< Last matched AutoPat
  size_t auidx;             ///< Current autocmd index to execute
  size_t ausize;            ///< Saved AutoCmd vector size
  char *afile_orig;         ///< Unexpanded <afile>
  char *fname;              ///< Fname to match with
  char *sfname;             ///< Sfname to match with
  char *tail;               ///< Tail of fname
  int group;                ///< Group being used
  event_T event;            ///< Current event
  sctx_T script_ctx;        ///< Script context where it is defined
  int arg_bufnr;            ///< Initially equal to <abuf>, set to zero when buf is deleted
  Object *data;             ///< Arbitrary data
  AutoPatCmd *next;         ///< Chain of active apc-s for auto-invalidation
};

typedef kvec_t(AutoCmd) AutoCmdVec;

typedef struct {
  event_T event;
  char *fname;
  char *fname_io;
  buf_T buf;
} SafeAutocommandEvent;


Review comment (bug):
Never take `buf_T` by value, it can cause all sorts of problems. `apply_autocmds` takes it by pointer anyway.

However, both approaches are wrong here because autocommands may free the buffer and invalidate pointers. [Take it by handle instead](https://github.com/neovim/neovim/issues/35366#issuecomment-3226437401) and use that handle to retrieve the `buf_T *` if it's valid (e.g: `find_buffer_by_handle`). If invalid, don't fire the event (maybe for some events it'll make sense to fallback to `curbuf` in that case; something to consider for later).

Code after:

/// Struct used to keep status while executing autocommands for an event.
typedef struct AutoPatCmd_S AutoPatCmd;
struct AutoPatCmd_S {
  AutoPat *lastpat;         ///< Last matched AutoPat
  size_t auidx;             ///< Current autocmd index to execute
  size_t ausize;            ///< Saved AutoCmd vector size
  char *afile_orig;         ///< Unexpanded <afile>
  char *fname;              ///< Fname to match with
  char *sfname;             ///< Sfname to match with
  char *tail;               ///< Tail of fname
  int group;                ///< Group being used
  event_T event;            ///< Current event
  sctx_T script_ctx;        ///< Script context where it is defined
  int arg_bufnr;            ///< Initially equal to <abuf>, set to zero when buf is deleted
  Object *data;             ///< Arbitrary data
  AutoPatCmd *next;         ///< Chain of active apc-s for auto-invalidation
};

typedef kvec_t(AutoCmd) AutoCmdVec;

typedef struct {
  event_T event;
  char *fname;
  char *fname_io;
  Buffer buf;
  int group;
  exarg_T *eap;
  Object *data;
} AutoCmdEvent;  // Used for "deferred" events, but can represent any event.
Repository: neovim/neovim
File: src/nvim/state.c (C)

Code before:
  return stuff_empty()
         && typebuf.tb_len == 0
         && !using_script()
         && !global_busy
         && !debug_mode;
}

/// Trigger SafeState if currently in a safe state, that is "safe" is true and
/// there is no typeahead.
void may_trigger_safestate(bool safe)
{
  bool is_safe = safe && is_safe_now();

  if (was_safe != is_safe) {
    // Only log when the state changes, otherwise it happens at nearly
    // every key stroke.
    DLOG(is_safe ? "SafeState: Start triggering" : "SafeState: Stop triggering");
  }
  if (is_safe) {
    SafeAutocommandEvent *evdata = xmalloc(sizeof(SafeAutocommandEvent));
    evdata->event = EVENT_SAFESTATE;
    evdata->fname = NULL;
    evdata->fname_io = NULL;
    evdata->buf = *curbuf;
    multiqueue_put(safe_autocmd_events, safe_autocmd_event, evdata);
    multiqueue_process_events(safe_autocmd_events);
  }
  was_safe = is_safe;
}

/// Something changed which causes the state possibly to be unsafe, e.g. a
/// character was typed.  It will remain unsafe until the next call to
/// may_trigger_safestate().
void state_no_longer_safe(const char *reason)
{
  if (was_safe && reason != NULL) {
    DLOG("SafeState reset: %s", reason);
  }
  was_safe = false;
}

bool get_was_safe_state(void)
{
  return was_safe;
}


Review comment (suggestion):
Not much use to push here (unless it's to maintain the event order if more are pushed while processing the queue, but in that case `SafeState` should always just trigger for whatever `curbuf` is at that point, not what it was when it was enqueued).

Can just process the queue and fire `SafeState` via `apply_autocmds` after (or maybe it's better to do so before?) for whatever `curbuf` happens to be.

Presumably the queue will be processed in more places? [Doing it here is too limited](https://github.com/neovim/neovim/pull/34285#issuecomment-3194653155).

Code after:
  return stuff_empty()
         && typebuf.tb_len == 0
         && !using_script()
         && !global_busy
         && !debug_mode;
}

/// Trigger SafeState if currently in a safe state, that is "safe" is true and
/// there is no typeahead.
void may_trigger_safestate(bool safe)
{
  bool is_safe = safe && is_safe_now();

  if (was_safe != is_safe) {
    // Only log when the state changes, otherwise it happens at nearly
    // every key stroke.
    DLOG(is_safe ? "SafeState: Start triggering" : "SafeState: Stop triggering");
  }
  if (is_safe) {
    apply_autocmds(EVENT_SAFESTATE, NULL, NULL, false, curbuf);
  }
  was_safe = is_safe;
}

/// Something changed which causes the state possibly to be unsafe, e.g. a
/// character was typed.  It will remain unsafe until the next call to
/// may_trigger_safestate().
void state_no_longer_safe(const char *reason)
{
  if (was_safe && reason != NULL) {
    DLOG("SafeState reset: %s", reason);
  }
  was_safe = false;
}

bool get_was_safe_state(void)
{
  return was_safe;
}