🤪

次の行を読み込む関数を作成する

に公開8

課題の内容

42の規則上課題のPDFを掲載できないのですが、ざっくりいうと渡したファイルを1行ずつ読み込んでくれる関数です。
forswitchなどがコード規約で禁止されているため、一部モダンじゃない書き方をしています。
それ以外で「ここもっとスッキリかけそう」と言う箇所があればお知らせください。
また、使用できる標準関数はreadmallocおよびfreeのみです。
ft_で始まる関数は標準関数を自作したもので、挙動は同じになっているはずです、

ソースコード

get_next_line.h
#ifndef GET_NEXT_LINE_H
# define GET_NEXT_LINE_H
# define SUCCESS 1
# define END_OF_FILE 0
# define READ_ERROR -1

# include <unistd.h>
# include <stdlib.h>

int	get_next_line(int fd, char **line);
size_t	ft_strlen(const char *s);
char	*ft_strchr(char *s, int c);
void	*ft_memmove(void *dst, const void *src, size_t len);
char	*ft_substr(char const *s, unsigned int start, size_t len);

#endif
get_next_line.c
#include "get_next_line.h"

static char	*free_return(char *save)
{
	free(save);
	save = NULL;
	return (NULL);
}

static int	count_till_endl(char *s)
{
	int	i;

	i = 0;
	if (!s)
		return (0);
	while (s[i] && s[i] != '\n')
		i++;
	return (i);
}

static char	*gnl_strjoin(char const *s1, char const *s2)
{
	size_t	len1;
	size_t	len2;
	size_t	total_len;
	char	*ret;

	if (!s1 && !s2)
		return (NULL);
	len1 = ft_strlen((char *)s1);
	len2 = ft_strlen((char *)s2);
	total_len = len1 + len2;
	if (!(ret = (char *)malloc((total_len + 1) * sizeof(char))))
		return (NULL);
	ft_memmove(ret, s1, len1);
	ft_memmove(ret + len1, s2, len2);
	ret[total_len] = '\0';
	free((char *)s1);
	return (ret);
}

static char	*get_save(char *save)
{
	char	*ret;
	int	i;
	int	j;

	i = 0;
	j = 0;
	if (!save)
		return (NULL);
	while (save[i] != '\n')
	{
		if (!save[i])
			return (free_return(save));
		i++;
	}
	if (!(ret = malloc(sizeof(char) * ((ft_strlen(save) - i) + 1))))
		return (NULL);
	i++;
	while (save[i])
		ret[j++] = save[i++];
	ret[j] = '\0';
	free(save);
	return (ret);
}

int		get_next_line(int fd, char **line)
{
	int		read_res;
	char		*buf;
	static char	*save;

	read_res = SUCCESS;
	if (fd < 0 || !line || BUFFER_SIZE < 1 ||
	!(buf = (char *)malloc((size_t)BUFFER_SIZE + 1)))
		return (READ_ERROR);
	while (!(ft_strchr(save, '\n')) && read_res != 0)
	{
		if ((read_res = read(fd, buf, BUFFER_SIZE)) == READ_ERROR)
		{
			free(buf);
			return (READ_ERROR);
		}
		buf[read_res] = '\0';
		save = gnl_strjoin(save, buf);
	}
	free(buf);
	*line = ft_substr(save, 0, count_till_endl(save));
	save = get_save(save);
	return ((read_res == END_OF_FILE) ? END_OF_FILE : SUCCESS);
}
main.c
#include <stdio.h>
#include <stdlib.h>
#include <fcntl.h>

#include "get_next_line.h"

/*main.cは提出しないため、printfとか使ってもOKです*/
int	main(int argc, char **argv)
{
	int	fd;
	char	*line;

	if (argc == 1 || argc == 2)
	{
		fd = (argc == 1) ? 0 : open(argv[1], P_RDONLY);
		while (get_next_line(fd, &line) == 1)
		{
			printf("%s\n", line);
			free(line);
		}
		close(fd);
	}
	else
		return (2);
	system("leaks a.out");
	return (0);
}

Discussion

藤田望藤田望

ft_で始まる関数は標準関数を自作したもので、挙動は同じになっているはずです、

strchrの第1引数の型はconst char*の筈ですがft_strchrのそれが

char	*ft_strchr(char *s, int c);

char*となっているのはミスだと思います。
あと、Cの標準関数にsubstrは存在しないと思いますが記事のコードではft_substrというのが使用されており「ft_で始まる関数は標準関数を自作したもの」という説明とは矛盾しています。

char	*ft_substr(char const *s, unsigned int start, size_t len);

開始位置と長さを表す引数型でunsigned intsize_tが混用されているのは良いデザインではないですね。

藤田望藤田望

「ここもっとスッキリかけそう」と言う箇所があればお知らせください。

get_next_line.h
#ifndef GET_NEXT_LINE_H
# define GET_NEXT_LINE_H
# define SUCCESS 1
# define END_OF_FILE 0
# define READ_ERROR -1

# include <unistd.h>
# include <stdlib.h>

int	get_next_line(int fd, char **line);
size_t	ft_strlen(const char *s);
char	*ft_strchr(char *s, int c);
void	*ft_memmove(void *dst, const void *src, size_t len);
char	*ft_substr(char const *s, unsigned int start, size_t len);

#endif

get_next_line.hの中で、関数プロトタイプに使用してる型を定義しているヘッダをインクルードするのであれば分かりますが、そうでないヘッダ

# include <unistd.h>
# include <stdlib.h>

はインクルードすべきではないですね。
size_tを使いたいということであれば

# include <stddef.h>

で十分だと思います。
get_next_line.hでの関数プロトタイプはget_next_line.cで定義された関数にとどめるべきであり、それ以外の関数

size_t	ft_strlen(const char *s);
char	*ft_strchr(char *s, int c);
void	*ft_memmove(void *dst, const void *src, size_t len);
char	*ft_substr(char const *s, unsigned int start, size_t len);

は別のところで関数プロトタイプを定義するべきと思います。

size_t	ft_strlen(const char *s);
char	*ft_substr(char const *s, unsigned int start, size_t len);

const char *char const *が混在してますがひとつのプログラムであればどちらかに統一した方が良いですね。

藤田望藤田望
static char	*free_return(char *save)
{
	free(save);
	save = NULL;
	return (NULL);
}

save = NULL;は無意味なことやられてる印象ですが、解放したメモリブロックを格納したポインタ変数にNULLを格納したいということであれば

static char	*free_return(char **save)
{
	free(*save);
	*save = NULL;
	return (NULL);
}

とでもされたら良いかもしれないですね。

藤田望藤田望
static char	*gnl_strjoin(char const *s1, char const *s2)
{
	size_t	len1;
	size_t	len2;
	size_t	total_len;
	char	*ret;

	if (!s1 && !s2)
		return (NULL);
	len1 = ft_strlen((char *)s1);
	len2 = ft_strlen((char *)s2);
	total_len = len1 + len2;
	if (!(ret = (char *)malloc((total_len + 1) * sizeof(char))))
		return (NULL);
	ft_memmove(ret, s1, len1);
	ft_memmove(ret + len1, s2, len2);
	ret[total_len] = '\0';
	free((char *)s1);
	return (ret);
}

この関数の中の処理ではs1s2はどちらも有効な文字列を指してる想定で動作するものと思うので、

	if (!s1 && !s2)
		return (NULL);

&&||が正しいんじゃないでしょうか?
ft_strlenの引数型はconst char*なので、

	len1 = ft_strlen((char *)s1);
	len2 = ft_strlen((char *)s2);

↑のconst外しはまったく意味がないですね。

	free((char *)s1);

なんでかs1freeしてますが文字列の結合とは関係ないのでこの関数の外で行うべきですね。

藤田望藤田望
static int	count_till_endl(char *s)
{
	int	i;

	i = 0;
	if (!s)
		return (0);
	while (s[i] && s[i] != '\n')
		i++;
	return (i);
}

文字列の'\0'か'\n'を検索したいということであればすでに用意済みのft_strchrを使うべきではないですか?
文字列の添え字がintですが他方ではsize_tunsigned intを使用されておりポリシーを感じません。
仮引数sの指してる先は読んでるだけなので型はconst char*にすべきですね。

藤田望藤田望
static char	*get_save(char *save)
{
	char	*ret;
	int	i;
	int	j;

	i = 0;
	j = 0;
	if (!save)
		return (NULL);
	while (save[i] != '\n')
	{
		if (!save[i])
			return (free_return(save));
		i++;
	}
	if (!(ret = malloc(sizeof(char) * ((ft_strlen(save) - i) + 1))))
		return (NULL);
	i++;
	while (save[i])
		ret[j++] = save[i++];
	ret[j] = '\0';
	free(save);
	return (ret);
}

まずget_saveという関数名が意味が分かりません。

	while (save[i] != '\n')
	{
		if (!save[i])
			return (free_return(save));
		i++;
	}

'\n'を検索されたいのであればft_strchrを使うべきでは?
文字列の添え字がintで宜しくないのはcount_till_endlと同様です。

	while (save[i])
		ret[j++] = save[i++];
	ret[j] = '\0';

文字列コピーはstrcpy等使用するべきですね。ft_strlenft_strchrがあってft_strcpyがない理屈はないと思います。

	free(save);

定義済のfree_returnを使われてないのは一貫性に欠けますね。

藤田望藤田望
int		get_next_line(int fd, char **line)
{
	int		read_res;
	char		*buf;
	static char	*save;

	read_res = SUCCESS;
	if (fd < 0 || !line || BUFFER_SIZE < 1 ||
	!(buf = (char *)malloc((size_t)BUFFER_SIZE + 1)))
		return (READ_ERROR);
	while (!(ft_strchr(save, '\n')) && read_res != 0)
	{
		if ((read_res = read(fd, buf, BUFFER_SIZE)) == READ_ERROR)
		{
			free(buf);
			return (READ_ERROR);
		}
		buf[read_res] = '\0';
		save = gnl_strjoin(save, buf);
	}
	free(buf);
	*line = ft_substr(save, 0, count_till_endl(save));
	save = get_save(save);
	return ((read_res == END_OF_FILE) ? END_OF_FILE : SUCCESS);
}

savestatic変数であり初期化子がないためNULLで初期化されますが

	while (!(ft_strchr(save, '\n')) && read_res != 0)

それをft_strchrに渡しています。標準関数のstrchrは第1引数にNULL渡すことはできませんがそれを平気でやっていることから察するにft_strchrは記事の冒頭に説明されている

ft_で始まる関数は標準関数を自作したもので、挙動は同じになっているはずです、

とはなっておらず、count_till_endlの実装のように

	if (!s)
		return (0);

引数のNULL判定を行いNULLか何かを返すよう実装されていると思います。
標準関数の動作としては未定義動作となるためそのような実装も許されないわけではありませんが、バグの発見を遅らせ問題を先送りにする行為なので褒められたものではありません。また、NULLが渡されても動作することを前提に呼び出しを行ってしまうと標準関数との互換性も失われてしまうこととなります。

この関数はNULLを長さ0の文字列として扱う作りになっていますが、大変筋が悪いです。gnl_strjoinget_saveが中で呼んでいるmallocがメモリ取得に失敗してNULLを返した場合それはエラーとして扱われるべきですがこの関数ではNULLは長さ0の文字列として扱っているためエラーの検出がされずsaveの指していた文字列は捨てられメモリリークを起こした上で先へ進んでしまいます。

また、saveにメモリが割り当てられた状態でmallocreadがエラーを返した場合、

	!(buf = (char *)malloc((size_t)BUFFER_SIZE + 1)))
		return (READ_ERROR);
		if ((read_res = read(fd, buf, BUFFER_SIZE)) == READ_ERROR)
		{
			free(buf);
			return (READ_ERROR);
		}

saveを開放する処理がないためメモリリークとなります。
あとEOFに達した場合、

	*line = ft_substr(save, 0, count_till_endl(save));

*lineに長さ0の文字列を書き込むため、mainからのget_next_lineの使い方が

		fd = (argc == 1) ? 0 : open(argv[1], P_RDONLY);
		while (get_next_line(fd, &line) == 1)
		{
			printf("%s\n", line);
			free(line);
		}
		close(fd);

この通りではメモリリークが発生すると思います。
# P_RDONLYO_RDONLYのtypoでしょうか?

get_next_lineはファイル記述子を仮引数として受け取る作りになっていますが、動作をひとつのstatic変数に依存しているため複数のファイル記述子に対応できない構造になっているのは設計バグとも思います。

藤田望藤田望

自己レス:

あとEOFに達した場合、

*lineに長さ0の文字列を書き込むため、mainからのget_next_lineの使い方が

この通りではメモリリークが発生すると思います。

gcc 15.2に-fsanitize=addressを指定してコンパイル、実行し確認してみました。

#ifndef BUFFER_SIZE
#define BUFFER_SIZE 4096
#endif

#ifndef P_RDONLY
#define P_RDONLY O_RDONLY
#endif

を追加し、mainprintfの後にはfflush(stdout);を追加しています。
他、足りない関数は

引数のNULL判定を行いNULLか何かを返すよう実装されていると思います。

に従い以下の実装を加えています。

#include <string.h>

size_t ft_strlen(const char *s)
{
    return !s ? 0 : strlen(s);
}

char* ft_strchr(char *s, int c)
{
    return !s ? NULL : strchr(s, c);
}

void* ft_memmove(void *dst, const void *src, size_t len)
{
    return !dst || !src ? NULL : memmove(dst, src, len);
}

char* ft_substr(char const *s, unsigned int start, size_t len)
{
    if (!s) return NULL;
    char* r = calloc(len + 1, sizeof(char));
    if (!r) return NULL;
    strncpy(r, s + start, len);
    return r;
}

https://godbolt.org/z/fEsEoox57

=================================================================
==1==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 1 byte(s) in 1 object(s) allocated from:
#0 0x752ee2ebe5a3 in calloc (/opt/compiler-explorer/gcc-15.2.0/lib64/libasan.so.8+0x1215a3) (BuildId: 620b620e803590cdaf76f7a713f6c544a53408b6)
#1 0x000000401aee in ft_substr /app/example.c:166
#2 0x0000004017eb in get_next_line /app/example.c:110
#3 0x0000004019b3 in main /app/example.c:133
#4 0x752ee2a29d8f (/lib/x86_64-linux-gnu/libc.so.6+0x29d8f) (BuildId: 095c7ba148aeca81668091f718047078d57efddb)

SUMMARY: AddressSanitizer: 1 byte(s) leaked in 1 allocation(s).