Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

【軽PR】x_mallocの使用 & ファイル構成の変更 #112

Merged
merged 3 commits into from
Oct 21, 2021
Merged

Conversation

takumihara
Copy link
Owner

Purpose

ファイル名とか変えちゃったので、見づらいかと思いますが、コード自体は特に変更していないので、軽くApprove頂けたらと思います!

Effect

minishell.cなどからはparse.hだけをインクルードするので、parser内部の処理で使っているその他の関数を使えないようにしている。モジュールに分けて処理が見られるようになるので、初見の人でも見やすく・考えやすくなるのでは?

Memo

internalディレクトリは内部処理的な意味なので、このままでもいい気がしますが、ヘッダー名が悩み中です...

@takumihara takumihara requested a review from moromin October 20, 2021 23:48
Copy link
Collaborator

@moromin moromin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

internalとわけるのすごくいいと思います!
全体のコードを追いやすくなりますし。
コメントの確認おねがいします!

SRC_PATHS = $(CUR_PATH) $(PARSER_PATH) $(LEXER_PATH) $(AST_PATH) $(UTILS_PATH)
WRAPPER_PATH= ../../wrapper

SRC_PATHS = $(CUR_PATH) $(PARSER_PATH) $(LEXER_PATH) $(AST_PATH) $(UTILS_PATH) $(WRAPPER_PATH)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makefile的に、PARSER_PATHとかに../internal追加しなくてもinternalディレクトリ下のファイルってコンパイルできましたっけ?
クローンしてきてmake reしたら失敗したんですが、タクミさんの方ではコンパイルできました?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

すんません、コンパイルできるか確認してなかったです笑
修正したのでご確認お願いします!

@takumihara takumihara requested a review from moromin October 21, 2021 01:53
Copy link
Collaborator

@moromin moromin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

修正あざす!

@@ -10,7 +10,7 @@
#include <sys/ioctl.h>

#include "libft/libft.h"
#include "parser/parser.h"
#include "parser/parse.h"
#include "lexer/lexer.h"
#include "expander/expander.h"
#include "execute/execute.h"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ディレクトリとヘッダ名は[verb-er]/[verb]の形で統一します?

Copy link
Owner Author

@takumihara takumihara Oct 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parse.hで宣言する関数がparse()だけだったので、parse.hにしました!
それで大丈夫でそうですか?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

個人的には、ディレクトリ名とヘッダ名を同じにした方が見やすい感はありますね

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

関数名は動詞の方がいいですよね?lex()``parse()みたいな

@takumihara takumihara merged commit b3ef330 into master Oct 21, 2021
@takumihara takumihara deleted the parser branch October 21, 2021 02:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants