Skip to content

Conversation

@dayano74
Copy link
Collaborator

@dayano74 dayano74 commented Apr 24, 2025

compile errorを解消するために余計なファイルをプッシュしています。

./test_create_envpするとenvpと一致しているかどうかを確かめられます。

@cacapon
Copy link
Owner

cacapon commented Apr 25, 2025

私の方でminishellの方のコンパイルエラーと最新Mainのマージ対応はしました。(何ヶ所が出ていたので…)
修正箇所については 5786bfbを確認してください。

それを踏まえてコードレビュー・動作確認をしました。

Copy link
Owner

@cacapon cacapon left a comment

Choose a reason for hiding this comment

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

テストコードの確認&コードレビューしました。
ちょっと気になるところがあるので、確認、回答及び修正をお願いします〜

それ以外の部分についてはいい感じに作れていてOKかなと思います!


#include "main.h"

int count_value(t_minish *minish)
Copy link
Owner

Choose a reason for hiding this comment

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

ここでしか使われていないならstaticつけた方がいいかもです。

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

そうですね。修正します

envp = create_envp(minish);
if (!envp)
{
perror("create_envp");
Copy link
Owner

Choose a reason for hiding this comment

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

perrorって確かエラー出力するだけなので、これだと後続の処理が実行されると思いますがこれは想定通りの動作ですか?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

あ、後ほど作業しようと思って忘れていました。修正します

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

command not foundを出力させて、t_cmdのstausに終了ステータスを入力する処理に修正しました。

Copy link
Owner

Choose a reason for hiding this comment

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

command not foundを出力させて、t_cmdのstausに終了ステータスを入力する処理に修正しました。

ありがとうございます。
ただ、Githubを見る限りだとコミットが私の対応が最新のままになっているので
もう一度pushしてもらってもいいですか?


#include "main.h"

static bool is_unit_builtin(t_cmd *cmd_head)
Copy link
Owner

Choose a reason for hiding this comment

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

今後対応予定の関数でしたら以下のように、コメントで書いておくと後でわかりやすいかなと思います。

/**
 * @brief 関数の概要
 * @param <...>
 * @return bool
 * @note TODO: 後で実装予定
 */

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

分かりました!

return (true);
}

static int exec_unit_builtin(t_cmd *cmd_head)
Copy link
Owner

Choose a reason for hiding this comment

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

こちらも上と同様コメントしておくとわかりやすいかと

/**
 * @brief 関数の概要
 * @param <...>
 * @return bool
 * @note TODO: 後で実装予定
 */

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

分かりました!

@cacapon
Copy link
Owner

cacapon commented Apr 25, 2025

修正確認できましたら、再度確認しますね〜

@dayano74
Copy link
Collaborator Author

dayano74 commented Apr 25, 2025

修正確認できましたら、再度確認しますね〜

おまたせしました。修正してプッシュしました。
確認お願いしましす。

@cacapon
Copy link
Owner

cacapon commented Apr 25, 2025

修正確認できましたら、再度確認しますね〜

おまたせしました。修正してプッシュしました。 確認お願いしましす。

create_envpがTOO MANY LINESでてるっぽいので対応お願いします💦

@cacapon
Copy link
Owner

cacapon commented Apr 25, 2025

マージで出たっぽいのでちょっとこっちでも見てみますね。

@cacapon
Copy link
Owner

cacapon commented Apr 25, 2025

対応ありがとうございます、コードは良さげなので、最低限の動作確認できたらApprove出します。

@dayano74
Copy link
Collaborator Author

マージで出たっぽいのでちょっとこっちでも見てみますね。

norm error修正したので確認お願いします。

@dayano74
Copy link
Collaborator Author

対応ありがとうございます、コードは良さげなので、最低限の動作確認できたらApprove出します。

ありがとうございます

Copy link
Owner

@cacapon cacapon left a comment

Choose a reason for hiding this comment

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

OKです!
ボリューム大きい対応でしたがお疲れ様でした!

@cacapon cacapon merged commit 1a7de1e into main Apr 25, 2025
1 check passed
@cacapon cacapon deleted the feature/create-envp branch April 25, 2025 12:27
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.

3 participants