2021年12月02日 : GitHubでPRを出してマージされた話

初PRがマージされた

GitHubのアカウントを作って久しいですが、実は一度もプルリクエストは出したことがありませんでした。巨大な人気プロジェクトだと特にバグとかみつけないし、新しめのプロジェクトとかはバグ出す前に使わなくなっちゃったりだし、機能追加のプルリクエストなんて早々に出来無いし……というのが理由でしょうか。

そんな中、最近仕事で(何を血迷ったのか)C++つかってツールを作っていて、その際に固定小数の扱いに困り、調べて辿りついたのが以下のプロジェクト。

これの固定小数のコードが、ヘッダファイル一つ持ってくるだけで使えたので、ちょっと使わせてもらっていました。が、どうもツールがうまく動かないと調べていたら、固定小数をunsigned型に変換するメソッドがとんでもなくでかい値を返してくれるという問題がある事に気付きました。コードは以下。

    constexpr unsigned int to_uint() const {
        return (data_ & integer_mask) >> fractional_bits;
    }

カンの良い方はすぐわかったかと思いますが、 date_ メンバはsignedです。そう、整数部をマスクして右シフトするのは良いのですが、 msbが立っていると符号拡張されてえらい値になってしまうのでした。

ということで、手元で作ったパッチが以下。

iff --git a/fixed/include/cpp-utilities/fixed.h b/fixed/include/cpp-utilities/fixed.h
index c4390d5..b3a11df 100644
--- a/fixed/include/cpp-utilities/fixed.h
+++ b/fixed/include/cpp-utilities/fixed.h
@@ -444,7 +444,7 @@ public: // conversion to basic types
        }
 
        constexpr unsigned int to_uint() const {
-               return (data_ & integer_mask) >> fractional_bits;
+               return static_cast<unsigned int>(data_ & integer_mask) >> fractional_bits;
        }
 
        constexpr float to_float() const {

まぁ、色々と回避策はありますが、先にunsignedにキャストしちゃうのが楽ですかね……。ということで、手元のツールが動いたのでめでたしめでたし……となりかけたのですが、よく考えたら、これは教えてあげたほうがよろしかろう……と。

で、特にREADMEにcontributeの関する細かいルールはなさそうなこと、単体テストは(完全ではないけど)用意されている、という感じだったので以下の簡単なテストを追加。

diff --git a/fixed/test/fixed.cpp b/fixed/test/fixed.cpp
index c5f4611..1951a1d 100644
--- a/fixed/test/fixed.cpp
+++ b/fixed/test/fixed.cpp
@@ -59,4 +59,7 @@ int main() {
        static_assert(0 !=   fixed{1}, "");
        static_assert(1 >=   fixed{0}, "");
        static_assert(0.5 <= fixed{1}, "");
+
+       // conversion test
+       assert(fixed(0x8000).to_uint() == 0x8000);
 }

なお、 fixed は32-bitで整数部16-bitな固定小数です。

ということで、上記のコードをPRを、仕事中にやるのはアレだったので、昨日の朝にTully'sでコーヒー水筒に入れてもらって優雅に(?)投げたところ、今日になってマージしてもらえました。

問題のPRはこちら。まぁ、今回のようにPR出しやすい不具合とかはそうそうないでしょうけど、これがGitHubの醍醐味の一つなのかなぁ……と思いましたとさ。

ところで、良い感じにGitHubのリンクのカード作るには、どうするのが良いんですかね。プロジェクトへのリンクは https://gh-card.dev/ というのがあったけど(これも使い勝手はあまりよくない)。

本日のタグ

GitHub