IntenseFinance/neovim
3
0
Fork 0
mirror of https://github.com/neovim/neovim.git synced 2025-02-25 18:55:25 -06:00
Code Issues Packages Projects Releases Wiki Activity

refactor(fileio): remove API shell layer encouraging unnecessary allocations

Browse Source 
Functions like file_open_new() and file_open_fd_new() which just is a
wrapper around the real functions but with an extra xmalloc/xfree around
is an anti-pattern. If the caller really needs to allocate a
FileDescriptor as a heap object, it can do that directly.

FileDescriptor by itself is pretty much a pointer, or rather two:
the OS fd index and a pointer to a buffer. So most of the time an extra
pointer layer is just wasteful.

In the case of scriptin[curscript] in getchar.c, curscript used
to mean in practice:

N+1 open scripts           when curscript>0
zero or one open scripts   when curscript==0

Which means scriptin[0] had to be compared to NULL to disambiguate the
curscript=0 case.

Instead, use curscript==-1 to mean that are no script,
then all pointer comparisons dissappear and we can just use an array of
structs without extra pointers.
This commit is contained in:
bfredl 2024-02-24 10:17:20 +01:00
parent 0fcbda5987
commit 77e928fd3e
7 changed files with 86 additions and 198 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

65
src/nvim/getchar.c
View File

@ -65,8 +65,9 @@
#include "nvim/vim_defs.h"
/// Index in scriptin
static int curscript = 0;
FileDescriptor *scriptin[NSCRIPT] = { NULL };
static int curscript = -1;
/// Streams to read script from
static FileDescriptor scriptin[NSCRIPT] = { 0 };
// These buffers are used for storing:
// - stuffed characters: A command that is translated into another command.
@ -1176,7 +1177,7 @@ void ungetchars(int len)
void may_sync_undo(void)
{
if ((!(State & (MODE_INSERT | MODE_CMDLINE)) || arrow_used)
&& scriptin[curscript] == NULL) {
&& curscript < 0) {
u_sync(false);
}
}
@ -1216,8 +1217,9 @@ void free_typebuf(void)
/// restored when "file" has been read completely.
static typebuf_T saved_typebuf[NSCRIPT];
void save_typebuf(void)
static void save_typebuf(void)
{
assert(curscript >= 0);
init_typebuf();
saved_typebuf[curscript] = typebuf;
alloc_typebuf();
@ -1292,18 +1294,13 @@ void openscript(char *name, bool directly)
return;
}
if (scriptin[curscript] != NULL) { // already reading script
curscript++;
}
curscript++;
// use NameBuff for expanded name
expand_env(name, NameBuff, MAXPATHL);
int error;
if ((scriptin[curscript] = file_open_new(&error, NameBuff,
kFileReadOnly, 0)) == NULL) {
int error = file_open(&scriptin[curscript], NameBuff, kFileReadOnly, 0);
if (error) {
semsg(_(e_notopen_2), name, os_strerror(error));
if (curscript) {
curscript--;
}
curscript--;
return;
}
save_typebuf();
@ -1330,7 +1327,7 @@ void openscript(char *name, bool directly)
update_topline_cursor(); // update cursor position and topline
normal_cmd(&oa, false); // execute one command
vpeekc(); // check for end of file
} while (scriptin[oldcurscript] != NULL);
} while (curscript >= oldcurscript);
State = save_State;
msg_scroll = save_msg_scroll;
@ -1342,31 +1339,53 @@ void openscript(char *name, bool directly)
/// Close the currently active input script.
static void closescript(void)
{
assert(curscript >= 0);
free_typebuf();
typebuf = saved_typebuf[curscript];
file_free(scriptin[curscript], false);
scriptin[curscript] = NULL;
if (curscript > 0) {
curscript--;
}
file_close(&scriptin[curscript], false);
curscript--;
}
#if defined(EXITFREE)
void close_all_scripts(void)
{
while (scriptin[0] != NULL) {
while (curscript >= 0) {
closescript();
}
}
#endif
bool open_scriptin(char *scriptin_name)
FUNC_ATTR_NONNULL_ALL
{
assert(curscript == -1);
curscript++;
int error;
if (strequal(scriptin_name, "-")) {
error = file_open_stdin(&scriptin[0]);
} else {
error = file_open(&scriptin[0], scriptin_name,
kFileReadOnly|kFileNonBlocking, 0);
}
if (error) {
fprintf(stderr, _("Cannot open for reading: \"%s\": %s\n"),
scriptin_name, os_strerror(error));
curscript--;
return false;
}
save_typebuf();
return true;
}
/// Return true when reading keys from a script file.
int using_script(void)
FUNC_ATTR_PURE
{
return scriptin[curscript] != NULL;
return curscript >= 0;
}
/// This function is called just before doing a blocking wait. Thus after
@ -2801,10 +2820,10 @@ int inchar(uint8_t *buf, int maxlen, long wait_time)
// Get a character from a script file if there is one.
// If interrupted: Stop reading script files, close them all.
ptrdiff_t read_size = -1;
while (scriptin[curscript] != NULL && read_size <= 0 && !ignore_script) {
while (curscript >= 0 && read_size <= 0 && !ignore_script) {
char script_char;
if (got_int
|| (read_size = file_read(scriptin[curscript], &script_char, 1)) != 1) {
|| (read_size = file_read(&scriptin[curscript], &script_char, 1)) != 1) {
// Reached EOF or some error occurred.
// Careful: closescript() frees typebuf.tb_buf[] and buf[] may
// point inside typebuf.tb_buf[]. Don't use buf[] after this!

3
src/nvim/getchar.h
View File

@ -17,9 +17,6 @@ typedef enum {
enum { NSCRIPT = 15, }; ///< Maximum number of streams to read script from
/// Streams to read script from
extern FileDescriptor *scriptin[NSCRIPT];
#ifdef INCLUDE_GENERATED_DECLARATIONS
# include "getchar.h.generated.h"
#endif

10
src/nvim/lua/executor.c
View File

@ -1828,7 +1828,11 @@ bool nlua_exec_file(const char *path)
lua_getglobal(lstate, "loadfile");
lua_pushstring(lstate, path);
} else {
FileDescriptor *stdin_dup = file_open_stdin();
FileDescriptor stdin_dup;
int error = file_open_stdin(&stdin_dup);
if (error) {
return false;
}
StringBuilder sb = KV_INITIAL_VALUE;
kv_resize(sb, 64);
@ -1837,7 +1841,7 @@ bool nlua_exec_file(const char *path)
if (got_int) { // User canceled.
return false;
}
ptrdiff_t read_size = file_read(stdin_dup, IObuff, 64);
ptrdiff_t read_size = file_read(&stdin_dup, IObuff, 64);
if (read_size < 0) { // Error.
return false;
}
@ -1849,7 +1853,7 @@ bool nlua_exec_file(const char *path)
}
}
kv_push(sb, NUL);
file_free(stdin_dup, false);
file_close(&stdin_dup, false);
lua_getglobal(lstate, "loadstring");
lua_pushstring(lstate, sb.items);

45
src/nvim/main.c
View File

@ -426,7 +426,19 @@ int main(int argc, char **argv)
params.edit_type = EDIT_STDIN;
}
open_script_files(&params);
if (params.scriptin) {
if (!open_scriptin(params.scriptin)) {
os_exit(2);
}
}
if (params.scriptout) {
scriptout = os_fopen(params.scriptout, params.scriptout_append ? APPENDBIN : WRITEBIN);
if (scriptout == NULL) {
fprintf(stderr, _("Cannot open for script output: \""));
fprintf(stderr, "%s\"\n", params.scriptout);
os_exit(2);
}
}
nlua_init_defaults();
@ -1620,37 +1632,6 @@ static void read_stdin(void)
check_swap_exists_action();
}
static void open_script_files(mparm_T *parmp)
{
if (parmp->scriptin) {
int error;
if (strequal(parmp->scriptin, "-")) {
FileDescriptor *stdin_dup = file_open_stdin();
scriptin[0] = stdin_dup;
} else {
scriptin[0] = file_open_new(&error, parmp->scriptin,
kFileReadOnly|kFileNonBlocking, 0);
if (scriptin[0] == NULL) {
vim_snprintf(IObuff, IOSIZE,
_("Cannot open for reading: \"%s\": %s\n"),
parmp->scriptin, os_strerror(error));
fprintf(stderr, "%s", IObuff);
os_exit(2);
}
}
save_typebuf();
}
if (parmp->scriptout) {
scriptout = os_fopen(parmp->scriptout, parmp->scriptout_append ? APPENDBIN : WRITEBIN);
if (scriptout == NULL) {
fprintf(stderr, _("Cannot open for script output: \""));
fprintf(stderr, "%s\"\n", parmp->scriptout);
os_exit(2);
}
}
}
// Create the requested number of windows and edit buffers in them.
// Also does recovery if "recoverymode" set.
static void create_windows(mparm_T *parmp)

66
src/nvim/os/fileio.c
View File

@ -129,59 +129,15 @@ int file_open_fd(FileDescriptor *const ret_fp, const int fd, const int flags)
return 0;
}
/// Like file_open(), but allocate and return ret_fp
///
/// @param[out] error Error code, or 0 on success. @see os_strerror()
/// @param[in] fname File name to open.
/// @param[in] flags Flags, @see FileOpenFlags.
/// @param[in] mode Permissions for the newly created file (ignored if flags
/// does not have kFileCreate\*).
///
/// @return [allocated] Opened file or NULL in case of error.
FileDescriptor *file_open_new(int *const error, const char *const fname, const int flags,
const int mode)
FUNC_ATTR_NONNULL_ALL FUNC_ATTR_WARN_UNUSED_RESULT
{
FileDescriptor *const fp = xmalloc(sizeof(*fp));
if ((*error = file_open(fp, fname, flags, mode)) != 0) {
xfree(fp);
return NULL;
}
return fp;
}
/// Like file_open_fd(), but allocate and return ret_fp
///
/// @param[out] error Error code, or 0 on success. @see os_strerror()
/// @param[in] fd File descriptor to wrap.
/// @param[in] flags Flags, @see FileOpenFlags.
/// @param[in] mode Permissions for the newly created file (ignored if flags
/// does not have FILE_CREATE\*).
///
/// @return [allocated] Opened file or NULL in case of error.
FileDescriptor *file_open_fd_new(int *const error, const int fd, const int flags)
FUNC_ATTR_NONNULL_ALL FUNC_ATTR_MALLOC FUNC_ATTR_WARN_UNUSED_RESULT
{
FileDescriptor *const fp = xmalloc(sizeof(*fp));
if ((*error = file_open_fd(fp, fd, flags)) != 0) {
xfree(fp);
return NULL;
}
return fp;
}
/// Opens standard input as a FileDescriptor.
FileDescriptor *file_open_stdin(void)
FUNC_ATTR_MALLOC FUNC_ATTR_WARN_UNUSED_RESULT
int file_open_stdin(FileDescriptor *fp)
FUNC_ATTR_WARN_UNUSED_RESULT
{
int error;
FileDescriptor *const stdin_dup = file_open_fd_new(&error, os_open_stdin_fd(),
kFileReadOnly|kFileNonBlocking);
assert(stdin_dup != NULL);
int error = file_open_fd(fp, os_open_stdin_fd(), kFileReadOnly|kFileNonBlocking);
if (error != 0) {
ELOG("failed to open stdin: %s", os_strerror(error));
}
return stdin_dup;
return error;
}
/// Close file and free its buffer
@ -202,20 +158,6 @@ int file_close(FileDescriptor *const fp, const bool do_fsync)
return flush_error;
}
/// Close and free file obtained using file_open_new()
///
/// @param[in,out] fp File to close.
/// @param[in] do_fsync If true, use fsync() to write changes to disk.
///
/// @return 0 or error code.
int file_free(FileDescriptor *const fp, const bool do_fsync)
FUNC_ATTR_NONNULL_ALL
{
const int ret = file_close(fp, do_fsync);
xfree(fp);
return ret;
}
/// Flush file modifications to disk
///
/// @param[in,out] fp File to work with.

36
src/nvim/shada.c
View File

@ -402,7 +402,7 @@ typedef ptrdiff_t (*ShaDaFileWriter)(ShaDaWriteDef *const sd_writer,
struct sd_write_def {
ShaDaFileWriter write; ///< Writer function.
ShaDaWriteCloser close; ///< Close function.
void *cookie; ///< Data describing object written to.
FileDescriptor cookie; ///< Data describing object written to.
const char *error; ///< Error message in case of error.
};
@ -643,7 +643,7 @@ static ptrdiff_t write_file(ShaDaWriteDef *const sd_writer, const void *const de
const size_t size)
FUNC_ATTR_NONNULL_ALL FUNC_ATTR_WARN_UNUSED_RESULT
{
const ptrdiff_t ret = file_write(sd_writer->cookie, dest, size);
const ptrdiff_t ret = file_write(&sd_writer->cookie, dest, size);
if (ret < 0) {
sd_writer->error = os_strerror((int)ret);
return -1;
@ -656,13 +656,14 @@ static void close_sd_reader(ShaDaReadDef *const sd_reader)
FUNC_ATTR_NONNULL_ALL
{
close_file(sd_reader->cookie);
xfree(sd_reader->cookie);
}
/// Wrapper for closing file descriptors opened for writing
static void close_sd_writer(ShaDaWriteDef *const sd_writer)
FUNC_ATTR_NONNULL_ALL
{
close_file(sd_writer->cookie);
close_file(&sd_writer->cookie);
}
/// Wrapper for read that reads to IObuff and ignores bytes read
@ -731,8 +732,6 @@ static ShaDaReadResult sd_reader_skip(ShaDaReadDef *const sd_reader, const size_
static int open_shada_file_for_reading(const char *const fname, ShaDaReadDef *sd_reader)
FUNC_ATTR_WARN_UNUSED_RESULT FUNC_ATTR_NONNULL_ALL
{
int error;
*sd_reader = (ShaDaReadDef) {
.read = &read_file,
.close = &close_sd_reader,
@ -740,9 +739,11 @@ static int open_shada_file_for_reading(const char *const fname, ShaDaReadDef *sd
.error = NULL,
.eof = false,
.fpos = 0,
.cookie = file_open_new(&error, fname, kFileReadOnly, 0),
.cookie = xmalloc(sizeof(FileDescriptor)),
};
if (sd_reader->cookie == NULL) {
int error = file_open(sd_reader->cookie, fname, kFileReadOnly, 0);
if (error) {
XFREE_CLEAR(sd_reader->cookie);
return error;
}
@ -754,7 +755,7 @@ static int open_shada_file_for_reading(const char *const fname, ShaDaReadDef *sd
/// Wrapper for closing file descriptors
static void close_file(void *cookie)
{
const int error = file_free(cookie, !!p_fs);
const int error = file_close(cookie, !!p_fs);
if (error != 0) {
semsg(_(SERR "System error while closing ShaDa file: %s"),
os_strerror(error));
@ -3003,6 +3004,7 @@ int shada_write_file(const char *const file, bool nomerge)
.error = NULL,
};
ShaDaReadDef sd_reader = { .close = NULL };
bool did_open_writer = false;
if (!nomerge) {
int error;
@ -3032,8 +3034,8 @@ int shada_write_file(const char *const file, bool nomerge)
// 3: If somebody happened to delete the file after it was opened for
// reading use u=rw permissions.
shada_write_file_open: {}
sd_writer.cookie = file_open_new(&error, tempname, kFileCreateOnly|kFileNoSymlink, perm);
if (sd_writer.cookie == NULL) {
error = file_open(&sd_writer.cookie, tempname, kFileCreateOnly|kFileNoSymlink, perm);
if (error) {
if (error == UV_EEXIST || error == UV_ELOOP) {
// File already exists, try another name
char *const wp = tempname + strlen(tempname) - 1;
@ -3054,6 +3056,8 @@ shada_write_file_open: {}
semsg(_(SERR "System error while opening temporary ShaDa file %s "
"for writing: %s"), tempname, os_strerror(error));
}
} else {
did_open_writer = true;
}
}
if (nomerge) {
@ -3076,16 +3080,16 @@ shada_write_file_nomerge: {}
}
*tail = tail_save;
}
int error;
sd_writer.cookie = file_open_new(&error, fname, kFileCreate|kFileTruncate,
0600);
if (sd_writer.cookie == NULL) {
int error = file_open(&sd_writer.cookie, fname, kFileCreate|kFileTruncate, 0600);
if (error) {
semsg(_(SERR "System error while opening ShaDa file %s for writing: %s"),
fname, os_strerror(error));
} else {
did_open_writer = true;
}
}
if (sd_writer.cookie == NULL) {
if (!did_open_writer) {
xfree(fname);
xfree(tempname);
if (sd_reader.cookie != NULL) {
@ -3132,7 +3136,7 @@ shada_write_file_nomerge: {}
|| old_info.stat.st_gid != getgid()) {
const uv_uid_t old_uid = (uv_uid_t)old_info.stat.st_uid;
const uv_gid_t old_gid = (uv_gid_t)old_info.stat.st_gid;
const int fchown_ret = os_fchown(file_fd(sd_writer.cookie),
const int fchown_ret = os_fchown(file_fd(&sd_writer.cookie),
old_uid, old_gid);
if (fchown_ret != 0) {
semsg(_(RNERR "Failed setting uid and gid for file %s: %s"),

59
test/unit/os/fileio_spec.lua
View File

@ -55,24 +55,12 @@ local function file_open(fname, flags, mode)
return ret1, ret2
end
local function file_open_new(fname, flags, mode)
local ret1 = ffi.new('int[?]', 1, { 0 })
local ret2 = ffi.gc(m.file_open_new(ret1, fname, flags, mode), nil)
return ret1[0], ret2
end
local function file_open_fd(fd, flags)
local ret2 = ffi.new('FileDescriptor')
local ret1 = m.file_open_fd(ret2, fd, flags)
return ret1, ret2
end
local function file_open_fd_new(fd, flags)
local ret1 = ffi.new('int[?]', 1, { 0 })
local ret2 = ffi.gc(m.file_open_fd_new(ret1, fd, flags), nil)
return ret1[0], ret2
end
local function file_write(fp, buf)
return m.file_write(fp, buf, #buf)
end
@ -131,26 +119,6 @@ describe('file_open_fd', function()
end)
end)
describe('file_open_fd_new', function()
itp('can use file descriptor returned by os_open for reading', function()
local fd = m.os_open(file1, m.kO_RDONLY, 0)
local err, fp = file_open_fd_new(fd, m.kFileReadOnly)
eq(0, err)
eq({ #fcontents, fcontents }, { file_read(fp, #fcontents) })
eq(0, m.file_free(fp, false))
end)
itp('can use file descriptor returned by os_open for writing', function()
eq(nil, uv.fs_stat(filec))
local fd = m.os_open(filec, m.kO_WRONLY + m.kO_CREAT, 384)
local err, fp = file_open_fd_new(fd, m.kFileWriteOnly)
eq(0, err)
eq(4, file_write(fp, 'test'))
eq(0, m.file_free(fp, false))
eq(4, uv.fs_stat(filec).size)
eq('test', io.open(filec):read('*a'))
end)
end)
describe('file_open', function()
itp('can create a rwx------ file with kFileCreate', function()
local err, fp = file_open(filec, m.kFileCreate, 448)
@ -276,21 +244,6 @@ describe('file_open', function()
end)
end)
describe('file_open_new', function()
itp('can open a file read-only', function()
local err, fp = file_open_new(file1, 0, 384)
eq(0, err)
eq(false, fp.wr)
eq(0, m.file_free(fp, false))
end)
itp('fails to open an existing file with kFileCreateOnly', function()
local err, fp = file_open_new(file1, m.kFileCreateOnly, 384)
eq(m.UV_EEXIST, err)
eq(nil, fp)
end)
end)
describe('file_close', function()
itp('can flush writes to disk also with true argument', function()
local err, fp = file_open(filec, m.kFileCreateOnly, 384)
@ -303,18 +256,6 @@ describe('file_close', function()
end)
end)
describe('file_free', function()
itp('can flush writes to disk also with true argument', function()
local err, fp = file_open_new(filec, m.kFileCreateOnly, 384)
eq(0, err)
local wsize = file_write(fp, 'test')
eq(4, wsize)
eq(0, uv.fs_stat(filec).size)
eq(0, m.file_free(fp, true))
eq(wsize, uv.fs_stat(filec).size)
end)
end)
describe('file_fsync', function()
itp('can flush writes to disk', function()
local err, fp = file_open(filec, m.kFileCreateOnly, 384)