Conversation
There was a problem hiding this comment.
Pull request overview
端末出力向けに ANSI エスケープシーケンスをまとめたカラー用ヘッダを追加し、フォント色・背景色の設定（固定色＋RGB指定）を利用できるようにする PR です。
Changes:
FontColor/BgColorを追加し、基本色と RGB 指定のエスケープ文字列を生成- フォント色用（38;2）と背景色用（48;2）の RGB コード生成を実装
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/include/color.hpp
Outdated
|
|
||
| class FontColor { | ||
| public: | ||
| static constexpr const char* CLEAR = "\033[0m"; |
There was a problem hiding this comment.
FontColor::CLEAR is set to "\033[0m", which resets all attributes (foreground, background, styles). If the intent is to clear only the font (foreground) color, use the ANSI default-foreground reset code ("\033[39m") or rename this constant to something like RESET_ALL to match its behavior.
src/include/color.hpp
Outdated
|
|
||
| class BgColor { | ||
| public: | ||
| static constexpr const char* CLEAR = "\033[0m"; |
There was a problem hiding this comment.
BgColor::CLEAR is set to "\033[0m", which resets all attributes (foreground, background, styles). If the intent is to clear only the background color, use the ANSI default-background reset code ("\033[49m") or rename this constant to something like RESET_ALL to match its behavior.
| static constexpr const char* CLEAR = "\033[0m"; | |
| static constexpr const char* CLEAR = "\033[49m"; |
| static std::string RGB(uint8_t R, uint8_t G, uint8_t B){ | ||
| std::stringstream buf; | ||
| buf << "\033[38;2;" << std::to_string(R) << ";" << std::to_string(G) << ";" << std::to_string(B) << "m"; | ||
|
|
||
| return buf.str(); |
There was a problem hiding this comment.
RGB() builds the escape sequence via std::stringstream and std::to_string, which incurs multiple temporary std::string allocations. Since you only write to the stream, prefer std::ostringstream and stream the numeric values directly (cast uint8_t to int) to reduce overhead, especially if this is used in hot paths.
| static std::string RGB(uint8_t R, uint8_t G, uint8_t B){ | ||
| std::stringstream buf; | ||
| buf << "\033[48;2;" << std::to_string(R) << ";" << std::to_string(G) << ";" << std::to_string(B) << "m"; | ||
|
|
||
| return buf.str(); |
There was a problem hiding this comment.
RGB() builds the escape sequence via std::stringstream and std::to_string, which incurs multiple temporary std::string allocations. Since you only write to the stream, prefer std::ostringstream and stream the numeric values directly (cast uint8_t to int) to reduce overhead, especially if this is used in hot paths.
| class FontColor { | ||
| public: | ||
| static constexpr const char* CLEAR = "\033[0m"; | ||
| static constexpr const char* BLACK = "\033[30m"; | ||
| static constexpr const char* RED = "\033[31m"; | ||
| static constexpr const char* GREEN = "\033[32m"; | ||
| static constexpr const char* YELLOW = "\033[33m"; | ||
| static constexpr const char* BLUE = "\033[34m"; | ||
| static constexpr const char* PURPLE = "\033[35m"; | ||
| static constexpr const char* CYAN = "\033[36m"; | ||
| static constexpr const char* WHITE = "\033[37m"; | ||
|
|
||
| static std::string RGB(uint8_t R, uint8_t G, uint8_t B){ | ||
| std::stringstream buf; | ||
| buf << "\033[38;2;" << std::to_string(R) << ";" << std::to_string(G) << ";" << std::to_string(B) << "m"; | ||
|
|
||
| return buf.str(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
FontColor and BgColor duplicate the same RGB formatting logic and the same CLEAR value. Consider extracting shared pieces (e.g., a small internal helper to format "\033[...;2;R;G;Bm") so changes (like fixing CLEAR semantics) don’t need to be applied in multiple places.
| static constexpr const char* CYAN = "\033[36m"; | ||
| static constexpr const char* WHITE = "\033[37m"; | ||
|
|
||
| static std::string RGB(uint8_t R, uint8_t G, uint8_t B){ |
There was a problem hiding this comment.
Parameter names in RGB() are uppercase (R/G/B), which is inconsistent with the lower-case parameter naming used elsewhere in this codebase (e.g., forward(const Matrix& in), backward(const Matrix& dout)). Consider renaming parameters to r/g/b for consistency.
No description provided.