株式会社クリアコード > ククログ

ククログ


ノータブルコード9 - C言語で文字列を扱うときはNULL終端されているかどうかに注意する

全文検索エンジンGroongaの他にPostgreSQLで高速に全文検索できる拡張PGroongaの開発にも参加している堀本です。
今回は、PGroongaの開発中に「注意しないと危ないな」と思ったコードを紹介します。

PGroongaはPostgreSQLの拡張として開発されています。
そのため、当然ですが、PostgreSQLとデータのやり取りを行います。
PostgreSQLには、PostgreSQLの型があり、以下のような型が組み込みの型として用意されています。

https://www.postgresql.jp/document/12/html/datatype.html#DATATYPE-TABLE

この中で文字列を格納する型としてよく使われるのがtext型(長さ制限のない可変長文字列)です。
今回PGroongaに新しい関数を実装するのに、以下のようにエラーログを出力する処理を書きました。

if (desc->natts <= i)
{
	ereport(ERROR,
		(errcode(ERRCODE_INTERNAL_ERROR),
		 errmsg("pgroonga: an invlid value was specified for column name: %s",
			 columnNameData)));
}

新しく実装した関数は以下のようなインターフェースを持っていて、PGroongaのインデックスを設定したカラムの名前を与えると、それに対応するPGroongaが内部で管理しているテーブルの名前を返すようになっています。

text pgroonga_index_column_name_string(indexName text, columnName text)

冒頭のコードは、この関数の第二引数に存在しないカラムの名前を指定された場合にエラーログを出力するようにしています。
ここで、ログに出力しているcolumnNameDataは、以下のようにtext型の変数からVARDATA_ANYマクロでデータを取り出しています。

const text *columnName = PG_GETARG_TEXT_PP(1);
const char *columnNameData = VARDATA_ANY(columnName);

エラーログで出力しているcolumnNameDataの方はchar *型なので、%sを使って出力するので問題ないように見えますが、PostgreSQLのtext型はNULL終端されていないので、%sを使って出力すると、意図しない領域まで出力してしまう可能性があります。
このようなバグを防ぐため、PostgreSQLのtext型を使用する場合は以下のように、必ず長さ指定をする必要があります。

if (desc->natts <= i)
{
	ereport(ERROR,
		(errcode(ERRCODE_INTERNAL_ERROR),
-		 errmsg("pgroonga: an invlid value was specified for column name: %s",
-			 columnNameData)));
+		 errmsg("pgroonga: an invlid value was specified for column name: %.*s",
+				(const int)columnNameSize,
+				columnNameData)));
}

C言語ではNULL終端の文字列として文字列を扱う場合(C言語の標準の文字列関数)とデータとデータの長さで文字列を扱う場合(printf%.*sを使う場合など)と文字列の開始位置のポインターと終了位置のポインターで文字列を扱う場合があり、扱いを間違うと思わぬバグを仕込むことになってしまいます。

C言語の文字列に関わる問題は多いですが、改めてC言語の文字列の扱いには注意が必要だと感じたコードの紹介でした。

2020-06-19

ノータブルコード8 - Viewをシンプルに保つ

全文検索エンジンGroongaの開発に参加している堀本です。
Groongaは、Redmineのプラグインとして組み込むことができます。名前を redmine_full_text_searchといいます。
このプラグインによって、Redmineの全文検索をGroongaを使って実行できます。

今回はこのプラグインへの機能追加した時の添削コミットを紹介します。

redmine_full_text_searchを使うと、検索結果からチケット等に遷移した際に、search_id, search_n というURLパラメータが付与された状態でチケット画面が表示されます。
これは、検索精度のチューニングのために付与している情報で、デフォルトで付与されるようになっています。
ただ、ユースケースによっては、これらのパラメーターが付与されたURLでは不都合なことがあります。
そのため、これらのパラメーターを無効にしたいという要望があり、それを実装しました。

実装が完了しPull Requestした当初のソースは以下のようになっていました。

      <% @result_set.each_with_index do |e, i| %>
        <dt class="<%= e.event_type %> icon icon-<%= e.event_type %>">
          <%= content_tag("span", e.project, :class => "project") unless @project == e.project %>
+         <% if fts_add_search_related_parameters_in_url? %>
          <%= link_to(e.event_highlighted_title,
                      e.event_url.merge("search_id" => @search_request.search_id,
                                        "search_n" => i + @search_request.offset),
                      :data => {:rank => e.rank}) %>
+         <% else %>
+         <%= link_to(e.event_highlighted_title, :data => {:rank => e.rank}) %>
+         <% end %>
        </dt>
        <dd>
          <ol class="search-snippets">

無効にするかどうかをオプションで選択できるようにしています。
<% if fts_add_search_related_parameters_in_url? %>の部分でパラメーターを付与するオプションが有効かどうかを判定し、有効だったらsearch_id, search_n をURLに付与するようにしています。
無効の場合は、<%= link_to(e.event_highlighted_title, :data => {:rank => e.rank}) %>が実行されてパラメーターが付与されないURLになります。

このコードは、プロジェクトのメンテナーによって以下のように添削されました。

上記のコードは、以下のようにURLパラメーターを付与するかどうかの判定を削除し、 search_result_entry_url(e, i)というメソッドを呼び出しその結果を表示するだけのコードになっています。

      <% @result_set.each_with_index do |e, i| %>
        <dt class="<%= e.event_type %> icon icon-<%= e.event_type %>">
          <%= content_tag("span", e.project, :class => "project") unless @project == e.project %>
-         <% if fts_add_search_related_parameters_in_url? %>
          <%= link_to(e.event_highlighted_title,
-                     e.event_url.merge("search_id" => @search_request.search_id,
-                                       "search_n" => i + @search_request.offset),
+                     search_result_entry_url(e, i),
                      :data => {:rank => e.rank}) %>
-         <% else %>
-         <%= link_to(e.event_highlighted_title, :data => {:rank => e.rank}) %>
-         <% end %>
        </dt>
        <dd>
          <ol class="search-snippets">

URLパラメーターを付与するかどうかの判定は別のファイルに以下のように定義されました。

  module Hooks
    module SearchHelper
      include FullTextSearch::Hooks::SettingsHelper
+
+     def search_result_entry_url(e, i)
+       if fts_enable_tracking?
+         search_parameters = {
+           "search_id" => @search_request.search_id,
+           "search_n" => i + @search_request.offset,
+         }
+       else
+         search_parameters = {}
+       end
+       e.event_url.merge(search_parameters)
+     end
    end
  end

添削前のコードでは、HTMLにオプションの有無を判定し、生成するURLを切り替える処理を埋め込んでいましたがこの処理を埋め込んだファイルは ERBテンプレートでした。

ERBテンプレートは、HTMLにRubyスクリプトを埋め込めるため、添削前のコミットのように条件を判定して表示を切り替えるというようなこともできます。

ただ、このようにすると、このファイルの見通しは悪くなります。
ERBテンプレートは、データをどうやって表示させるかを書く場所であるため、表示させるデータを選択する処理を埋め込むと表示を担当する処理と、表示するデータを選択する処理が混在してしまい、複雑なコードになりやすくなります。

Ruby on Rails には、こんな時のためにヘルパーという仕組みが用意されています。

添削後のコードでは、オプションの有無の判定はヘルパーに移動され、ERBテンプレートでは、ヘルパーの処理結果を表示するだけになりました。
これにより、ERBテンプレート内の判定処理がなくなり見通しがよくなりました。

見通しの良いコードはバグを埋め込みにくくなりますし、読みやすく理解もしやすくなります。理解しやすいコードはメンテナンスもしやすいです。
ファイルの役割分担を考えて実装することで、より良いコードになるということに改めて気がつく出来事でした。

2020-06-03

ノータブルコード7 - Rustのif式を賢く使う

組み込みGeckoプロジェクトでRustに本格的に触れ始めた畑ケです。
今回は、組み込みGeckoプロジェクトでフィードバックしたgit2-rsのコミットから「これは!」、と思ったコードを見つけたので紹介します。

まず、git2-rsというRustのcrate*1 は、libgit2というライブラリのRustバインディングです。git2-rsはRustのパッケージマネージャーのcargoの依存crateの一つで、cargoはRustのパッケージ情報を取得するときにgitの操作が必要になる場面があります。git2-rsというcrateはcargoに必要なgitの操作を担当します。

背景

git2-rsのCライブラリのバインディングを担当しているlibgit2-sysというcrateの中に、libgit2のエラーメッセージを取得する操作が書かれていなかったため、
cargoの中でgitの操作が失敗したときにはエラーコードしか報告されないという問題がありました。

そこで、次節のパッチを提出しました。

ノータブルコードとなる前のパッチ

diff --git a/libgit2-sys/lib.rs b/libgit2-sys/lib.rs
index eba2077..9d1afba 100644
--- a/libgit2-sys/lib.rs
+++ b/libgit2-sys/lib.rs
@@ -7,6 +7,8 @@ extern crate libz_sys as libz;
 use libc::{c_char, c_int, c_uchar, c_uint, c_void, size_t};
 #[cfg(feature = "ssh")]
 use libssh2_sys as libssh2;
+use std::ffi::CStr;
+use std::ptr;
 
 pub const GIT_OID_RAWSZ: usize = 20;
 pub const GIT_OID_HEXSZ: usize = GIT_OID_RAWSZ * 2;
@@ -3551,7 +3553,25 @@ pub fn init() {
         openssl_init();
         ssh_init();
         let r = git_libgit2_init();
-        assert!(r >= 0, "couldn't initialize the libgit2 library: {}", r);
+        if r < 0 {
+            let git_error = git_error_last();
+            let mut error_msg: *mut c_char = ptr::null_mut();
+            if !git_error.is_null() {
+                error_msg = (*git_error).message;
+            }
+            if !error_msg.is_null() {
+                assert!(
+                    r >= 0,
+                    "couldn't initialize the libgit2 library: {}, error: {}",
+                    r,
+                    CStr::from_ptr(error_msg).to_string_lossy()
+                );
+            } else {
+                assert!(r >= 0, "couldn't initialize the libgit2 library: {}", r);
+            }
+        } else {
+            assert!(r >= 0, "couldn't initialize the libgit2 library: {}", r);
+        }
 
         // Note that we intentionally never schedule `git_libgit2_shutdown` to
         // get called. There's not really a great time to call that and #276 has

特に以下の部分がこのパッチの要点です。

            let git_error = git_error_last();
            let mut error_msg: *mut c_char = ptr::null_mut();
            if !git_error.is_null() {
                error_msg = (*git_error).message;
            }

このパッチでは、一度error_msg変数を可変なものとして宣言し、条件分岐の中で値を変更するという書き方になっています。これはC言語のライブラリなどでよく見かける書き方です。

リファイン後のパッチ

前述のパッチは無事取り込まれましたが、当該箇所を改めてプロジェクトオーナー側でリファインされました。ミュータブルな変数を使うよりも変数束縛を使う方がRustらしいコードです。
その考えに則って修正されたパッチが以下の通りです。

diff --git a/libgit2-sys/lib.rs b/libgit2-sys/lib.rs
index 9d1afba840..a5998af84c 100644
--- a/libgit2-sys/lib.rs
+++ b/libgit2-sys/lib.rs
@@ -8,7 +8,6 @@ use libc::{c_char, c_int, c_uchar, c_uint, c_void, size_t};
 #[cfg(feature = "ssh")]
 use libssh2_sys as libssh2;
 use std::ffi::CStr;
-use std::ptr;
 
 pub const GIT_OID_RAWSZ: usize = 20;
 pub const GIT_OID_HEXSZ: usize = GIT_OID_RAWSZ * 2;
@@ -3552,30 +3551,25 @@ pub fn init() {
     INIT.call_once(|| unsafe {
         openssl_init();
         ssh_init();
-        let r = git_libgit2_init();
-        if r < 0 {
-            let git_error = git_error_last();
-            let mut error_msg: *mut c_char = ptr::null_mut();
-            if !git_error.is_null() {
-                error_msg = (*git_error).message;
-            }
-            if !error_msg.is_null() {
-                assert!(
-                    r >= 0,
-                    "couldn't initialize the libgit2 library: {}, error: {}",
-                    r,
-                    CStr::from_ptr(error_msg).to_string_lossy()
-                );
-            } else {
-                assert!(r >= 0, "couldn't initialize the libgit2 library: {}", r);
-            }
-        } else {
-            assert!(r >= 0, "couldn't initialize the libgit2 library: {}", r);
+        let rc = git_libgit2_init();
+        if rc >= 0 {
+            // Note that we intentionally never schedule `git_libgit2_shutdown`
+            // to get called. There's not really a great time to call that and
+            // #276 has some more info about how automatically doing it can
+            // cause problems.
+            return;
         }
 
-        // Note that we intentionally never schedule `git_libgit2_shutdown` to
-        // get called. There's not really a great time to call that and #276 has
-        // some more info about how automatically doing it can cause problems.
+        let git_error = git_error_last();
+        let error = if !git_error.is_null() {
+            CStr::from_ptr((*git_error).message).to_string_lossy()
+        } else {
+            "unknown error".into()
+        };
+        panic!(
+            "couldn't initialize the libgit2 library: {}, error: {}",
+            rc, error
+        );
     });
 }
 

このパッチのコードの要点を以下に抜粋します。
ミュータブルな変数のerror_msgを削除し、代わりに以下のRustのコードでは、if式の性質を使いerrorという変数束縛にエラーメッセージを格納するコードになっています。

        let git_error = git_error_last();
        let error = if !git_error.is_null() {
            CStr::from_ptr((*git_error).message).to_string_lossy()
        } else {
            "unknown error".into()
        };

このように、Rustではifは文ではなく式なので、値を返すことができます。そのため、ifで分岐した時の結果を上記のように一つの変数束縛に入れることができます。
変数束縛を意識的に使うのがよりRustらしいコードとなるため紹介しました。

*1 Rustでは、ライブラリの事をcrateと呼びます。

2020-05-15

ノータブルコード6 - ポインターのサイズで32bit環境を見分ける

Groongaという全文検索エンジンの開発に参加している堀本です。
今回は、開発中にGroongaのソースコードから「おぉー」と思ったコードを見つけたので紹介します。

Groongaは32bit用パッケージと64bit用パッケージを配布していますが、32bit環境のときだけコンパイルオプション等を設定したいケースがあります。

例えば、32bitのWindows向けパッケージはCMakeを使用してビルドしていますが、CMakeには32bitのWindowsかどうかを判定できる定義済みの変数はありません。
WIN32という定義済みの変数がありますが、これはコンパイルターゲットが(64bitを含む)Windowsの場合にTrueになるので、32bitかどうかの判定には使えません。

そこでGroongaでは、以下のようにポインターのサイズを使って32bitかどうかを判定しています。
(CMAKE_SIZEOF_VOID_Pvoidポインタのサイズを計算してくれるCMakeの定義済みの変数です。)

  if(CMAKE_SIZEOF_VOID_P EQUAL 4)
    # 32bit
    list(APPEND MRUBY_DEFINITIONS "MRB_METHOD_T_STRUCT")
  endif()

データ型のサイズは、データ型モデルという定義にしたがって決まります。データ型モデルとはデータ型の大きさを定義したもので、OSによって異なりますが、概ね以下のどれかになります。

32bit用データ型モデル
データモデル名 short int long long long pointer
ILP32 16 32 32 64 32
LP32 16 16 32 64 32

64bit用データ型モデル
データモデル名 short int long long long pointer
LLP64 16 32 32 64 64
LP64 16 32 64 64 64
IP64 16 64 32 64 64
ILP64 16 64 64 64 64
SILP64 64 64 64 64 64

上記を見てわかるとおり、どのモデルであっても32bitの場合、ポインターのサイズは、32bit(4byte)になっています。
また、同様に64bitの場合も、ポインターのサイズはどのモデルも64bit(8byte)になっています。

現状出回っているほとんどの環境では、上記のように32bitと64bitでポインターのサイズは決まっているので、これを利用して32bit環境かどうかを判定できるのです。

様々なOSの32bit版のソフトウェアをメンテナンスしている方は、上記のような判定方法も利用してみてはいかがでしょうか?

2020-05-14

ノータブルコード5 - Lispとシェルスクリプトのキマイラ

今回紹介するコードは、エディンバラ大学が開発した音声合成ライブラリFestivalからのコードです。このライブラリには、テキストファイルを音声に変換する「text2wave」という実行ファイルが付属しています。このファイルの冒頭部分を引用すると次のようになっています。

#!/bin/sh
"true" ; exec /bin/festival --script $0 $*
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;-*-mode:scheme-*-
;;                                                                       ;;
;;                Centre for Speech Technology Research                  ;;
;;                     University of Edinburgh, UK                       ;;
;;                       Copyright (c) 1996,1997                         ;;
;;                        All Rights Reserved.                           ;;
...

;;; Because this is a --script type file I has to explicitly
;;; load the initfiles: init.scm and user's .festivalrc
(load (path-append libdir "init.scm"))

;;; Process command line arguments
(define (text2wave_help)
  (format t "%s\n"
  "text2wave [options] textfile
  Convert a textfile to a waveform
  Options

最初の2行を見て「ああこれはシェルスクリプトなんだな」と思っていると、突然3行目から Lisp/Scheme のコードが始まってビックリさせられます。それ以降のコードはSchemeのプログラムになっていて、シェルスクリプトの面影は影も形もなくなってしまいます。上の抜粋からも、ごく普通のシェルスクリプトの書き出しから、シームレスにSchemeのS式の世界に突入していることがご確認いただけると思います。

これはどういう仕組みで動いているんだろう?というのが、今回の記事のテーマです。

シェルスクリプトとしての実行プロセス

この謎を解き明かすカギは、最初の2行にあります。このスクリプトを「/bin/text2wave test.txt」として実行した時の流れを追ってみましょう。

#!/bin/sh
"true" ; exec /bin/festival --script $0 $*

まず、オペレーティングシステムのプログラムローダーは最初の1行目を見て「これは/bin/shで実行するコードだ」と判断します。ですので、このファイル全体が(最初の見立て通り)まずはシェルスクリプトとして解釈されることになります。

そこで2行目に目を移すと、シェルスクリプトではセミコロン ; はコマンドの区切りを示すので、この行は2つのコマンドから構成されていることが分かります。このうちの、前半部分は特に何もせず成功ステータスコードを返すtrueコマンドなので*1、実行だけされて次に移ります。

これに対して、後半部分にはもっと意味のありそうなコマンドが書かれています。まずこのコマンドの中のシェル変数に注目すると $0 は実行されているプログラムを表すので「/bin/text2wave」に、$* はコマンドライン引数を表すので「test.txt」にそれぞれ置換されます。いいかえると、この部分は次のようなコマンド行に展開されることになります。

exec /bin/festival --script /bin/text2wave test.txt

冒頭のexecはシェル標準のコマンドで、指定した実行プログラムで実行中のプロセスの内容を置き換えます。ですので、このコマンドが実行されると、シェルスクリプトとしての実行はひとまず終了になります。

Schemeスクリプトとしての評価

シェルスクリプトの最後で実行された /bin/festival に着目しましょう。これは、音声合成ライブラリの実行バイナリで、引数「--script」で与えられたファイルをSchemeスクリプトとして処理します。ここでは /bin/text2wave を指定しているので、最初にシェルスクリプトとして評価したスクリプトが(今度はSchemeのスクリプトとして)再び頭から解釈されることになります。

このSchemeインタプリタによる実行評価は次のように進行します。

  • まず、大抵のLisp処理系は#!をコメント行として扱います。
    • これは言語の仕様ではありませんが、スクリプト言語としての用途を考慮して、そのように実装されていることが多いです。
    • FestivalのSchemeインタプリタも同様の振る舞いをするので、1行目はコメントとして無視されます。
  • 2行目の前半部分については、Schemeでは"true"は文字列リテラルなので、評価だけして素通りします。
  • 2行目の後半部分は重要なポイントで、実はSchemeではセミコロンはコメントの開始を表します。 従って、execから行末までの部分はコメントとして無視されます。

そこから後の部分は普通のSchemeプログラムなので、普通に実行されることになります。

話をまとめると、このスクリプトは (1) シェルスクリプトとして実行することもできるし、(2) Schemeスクリプトとしても実行できるという実に不思議なプログラムなのです。この特筆すべき性質によって、Schemeとシェルスクリプトというまったく性質の異なる言語が、1つのファイルの中でなめらかに共存できているのです。

なぜこんなことをしているのか?

「確かに面白いテクニックだけど、なぜこんなことをしているの? 普通に最初の行で#!/bin/festival --scriptと指定するだけではダメなの?」というのは当然思い浮かぶ疑問です。このFestivalのコードは20年以上前のコードで、すでに実装の背景は分からなくなってしまってるのですが、この構成を採用するメリットは少なくとも2つあります。

  1. システム互換性の問題を回避できる。 Unixでは一般に「#!/path/to/program」という一行をスクリプトの冒頭につけることで、実行するプログラムを指定できますが、実は、この仕組みには明確な仕様がなく、具体的な振る舞いはシステムによってまちまちです(例えば、古いOSだと実行ファイルのパスの後に引数を指定できなかったりします)。この方式だと#!/bin/shさえ上手く解釈されれば、あとはシェルスクリプトで呼び出しを制御するので、非常に広い範囲のUnixシステムでポータブルに動作することが期待できます。
  2. 実行プログラムに渡す引数を柔軟に調整できる。 最終的に本体ファイル /bin/festival を呼び出すのはシェルスクリプトなので、実行時のパラメータをいかようにも制御できます。シェルの制御構文を使えば、条件分岐で与える引数を動的に変えることもできます。これは通常のやり方では実現できないポイントです。

まとめ

今回は音声合成ライブラリのFestivalから、Lispとシェルがなめらかに統合された、ギリシャ神話の怪物キマイラのようなコード(頭部はシェルスクリプトで胴体はSchemeです!)をご紹介しました。

皆さんはこのコードを読んでどのような感想を抱きましたか? ノータブルコードのコーナーでは皆さんの寄稿も受け付けていますので、ご意見や「これをぜひ紹介したい」というコードがあれば、ぜひともお寄せください。

*1  (3/17追記) 最初の公開版では「単なる文字列リテラルで、特に変数に代入もされていないので」と説明していましたが、コメントの指摘の通り、誤りのため修正しています。ご指摘ありがとうございます!

2020-03-10

ノータブルコード4 - NULLよりも名前付きの番兵オブジェクト

MroongaというMySQLのストレージエンジンを開発している須藤です。MySQLのAPIはよく変わるのでMroongaの開発をしているとMySQLのソースコードを読む機会がよくあります。今日、MySQLのコードを読んでいて「お!」と思うコードがあったので4回目のノータブルコードとして紹介します。

MySQLは基本的なデータ構造も独自に実装していることが多いです。今日紹介するListも独自に実装しています。

多くの場合、リストは次のように「要素データへのポインタ」と「次の要素へのポインタ」を持つ構造をつなげて実装します。

struct List {
  void *data;
  List *next;
};

そして、リストの終端にNULLを置いてリストの終わりを表現します。

if (list->next) {
  printf("have more elements\n");
} else {
  printf("no more elements\n");
}

MySQLのリストの実装はNULLではなくリストの終端を示す番兵オブジェクトを使っていました。

extern MYSQL_PLUGIN_IMPORT list_node end_of_list;

class base_list {
  inline void empty() {
    elements = 0;
    first = &end_of_list;
    last = &first;
  }
};

私が「お!」と思ったのはGDBでデバッグしていたときです。GDBではpで出力したアドレスが既知のグローバル変数や関数などの場合はその名前も出力してくれます。ここでend_of_listという名前が出てきたのです。

(gdb) p (((const Item_cond *)select_lex->where_cond())->argument_list()->first->next->next
$1 = (list_node *) 0x555557f77550 <end_of_list>
(gdb) p (list_node *)0x555557f77550
$2 = (list_node *) 0x555557f77550 <end_of_list>

->nextとしたらNULLが返ってきても「あぁ、ここでリストは終わりなんだな」ということはわかるのですが、end_of_listという名前が見えてもたしかにリストが終わりだとことがわかるなと思いました。リストのときはNULLで十分だとは思いますが、もう少し複雑なもののときはNULLよりもなにか名前が付いた番兵オブジェクトを使うとデバッグが捗るときがあるんじゃないかと思いました。このテクニックを使う機会を見つけることが楽しみです。

今回はMySQLのリスト実装のコードで「お!」と思った名前付きの番兵オブジェクトを紹介しました。みなさんもNULLではなく名前付きの番兵オブジェクトを使った方がよさそうなケースがないか考えてみてください。

ところで、そろそろみなさんも自分が「お!」と思ったコードを「ノータブルコード」として紹介してみたくなってきませんか?ということで、このブログへの寄稿も受け付けることにしました。まだ仕組みは整えていないのですが、とりあえず、 https://gitlab.com/clear-code/blog/issues にMarkdownっぽいマークアップで書いた原稿を投稿してもらえばいい感じに調整してここに載せます。寄稿したいという人がたくさんいるならもう少しちゃんとした仕組みを整えようと思っていますので、興味のある人はご連絡ください。寄稿してもらった記事の著作者は作者本人のままですが、ライセンスはCC BY-SA 4.0GFDL(バージョンなし、変更不可部分なし、表表紙テキストなし、裏表紙テキストなし)のデュアルライセンスにしてください。参考:ククログのライセンス

それでは、次のノータブルコードをお楽しみに!

2020-03-03

ノータブルコード3 - 危険なバグを仕組みで予防する

第三回目のノータブルコードで取り上げるのは、分散バージョン管理システムGitのヘッダファイル「banned.h」です。

banned.h とは何か?

これは何かと言うと、その名前の通り「危ない関数の利用を禁止する」ためのヘッダです。実際にコードを見てみましょう。

#ifndef BANNED_H
#define BANNED_H

/*
 * This header lists functions that have been banned from our code base,
 * because they're too easy to misuse (and even if used correctly,
 * complicate audits). Including this header turns them into compile-time
 * errors.
 */

#define BANNED(func) sorry_##func##_is_a_banned_function

#undef strcpy
#define strcpy(x,y) BANNED(strcpy)
#undef strcat
#define strcat(x,y) BANNED(strcat)
...

https://git.kernel.org/pub/scm/git/git.git/tree/banned.h

ここで注目したいのは、冒頭に定義されている BANNED というマクロです。そもそも、この banned.h がどうやって関数の使用を禁止しているかというと、「禁止したい関数の呼び出しを、まったく別の関数の呼び出しに書き換える」という方法でこれを実現しています。この書き換えを担当しているのが BANNED マクロで、例えば最初の strcpy を例にとると、これを sorry_strcpy_is_a_banned_function に強制的に書き換えてしまいます。

禁止された関数を使うとすると何が起きるのか?

実際にこのヘッダをインクルードして、禁止された関数を使ってみましょう。今回はこんなコードを用意しました。

#include <string.h>
#include "banned.h"

int main(int argc, char *argv[])
{
    char buf[16];
    strcpy(buf, "aiueo");
    return 0;
}

このコードをコンパイルすると何が起きるかというと、未定義の識別子エラーが発生してコンパイラがコケます。コンパイルが失敗する原因は sorry_strcpy_is_a_banned_function() という関数が(当然のことながら)存在しないためです。実際のエラーメッセージを以下に示します。

$ cc test.c
In file included from test.c:2:
test.c: In function ‘main’:
banned.h:11:22: error: ‘sorry_strcpy_is_a_banned_function’ undeclared (first use in this function)

ここで非常に面白いのは、BANNED マクロが書き換えた後の関数名が、そのままエラーメッセージになっていることです ("Sorry strcpy is a banned function") 。つまり、ここで BANNED マクロは (1) コンパイル処理を失敗させ (2) さらに失敗した理由を開発者に伝える、という一人二役を担っているのです。

ビルドシステムに組み込まれた安全

ヒューマンエラーを低減する為の最も効果的な設計技法の1つは、エラーが物理的に不可能であるか、あるいはエラーが明白なように設計することである。例えば、連結部分のサイズが異なるように設計すれば弁を取り違えることはできなくなる。あるいは、非対称、すなわちオス/メスの連結にすることで1つの方法でしか組み立てられないようにすることは可能である。カラーコーディングで接続の誤りを明示することができる。同等の技法を、オペレータとコンピュータのインターフェイス設計に利用することができる。

ナンシー・G・レブソン 『セーフウェア - 安全・安心なシステムとソフトウェアを目指して』(2009年, 翔泳社)p447

「システムの安全性をどのように実現するか?」という問題は、システム工学の中心的な課題の一つですが、その一つの知見に「ヒューマンエラーの大部分は、実は設計の欠陥である」というものがあります。単純に「人間のミス」として片付けてしまわず、ヒューマンエラーを引き起こした設計を分析して、システム面から対策を講じない限り、次の事故を防ぐことは困難なのです。

今回紹介した BANNED マクロは、システムに組み込まれた安全の一例です。strcpy()strcat() が深刻な不具合を生みやすい機能であることについては、長い議論の積み重ねがあります。ヒューマンエラー防止の観点から見ると、この BANNED マクロは次の3つの特長を持っています。

  1. コーディング規約のような約束ではなく、マクロによる書き換えという仕組みにより、危険な実装を防いでいる。
  2. 単に警告メッセージを出すだけではなく、規約に反するコードはコンパイルが失敗するようにしている。
  3. すべてのコンパイラでも動作する汎用的な制限ロジックを、独自に実装している。

このうち(3)は、手法面で興味深い部分です。例えば、特定のプラットフォーム(例えばGCC)を前提にしてよいなら、#pragma GCC poisonといったコンパイラ独自の機能を使うことで、同じ目的を達成することができます。これにはコンパイラの支援を受けられる(より詳細なエラーメッセージが出せるなど)というメリットがありますが、その一方で他のプラットフォームではうまく機能しなくなるという別の問題を抱えることになります。

今回の BANNED マクロは、コンパイラの支援を受けずに、原因を明解に示すエラーメッセージを出力しているという非常に面白い試みです。

まとめ

皆さんも、コーディング規約を一歩進めて、仕組みによってコードの健全性を担保するようにしてみませんか? 皆さんが使っている、効果的な工夫やアイデアがあれば、ぜひとも教えてください。

2020-02-12

ノータブルコード2 - 斬新なコメントでコードの可読性を高める

この新しいコーナー「ノータブルコード」では、私たちが開発の折々に目にした興味深いコードをご紹介しています。世の中の実際のプロジェクトから、興味深い素材を肩の凝らない形でご紹介していきたいと思いますので、楽しみにしていてください。

Brubeck - 創造的なコメントで見通しを良くする

第2回目に紹介するのは、GitHubが開発したStatsD互換サーバー Brubeck からの一コマです。

int brubeck_statsd_msg_parse(struct brubeck_statsd_msg *msg, char *buffer, char *end)
{
    *end = '\0';

    /**
     * Message key: all the string until the first ':'
     *
     *      gaugor:333|g
     *      ^^^^^^
     */
    {
        msg->key = buffer;
        msg->key_len = 0;
        while (*buffer != ':' && *buffer != '\0') {
            /* Invalid metric, can't have a space */
            if (*buffer == ' ')
                return -1;
            ++buffer;
    ...

https://github.com/github/brubeck/blob/master/src/samplers/statsd.c#L140-L241

このコードが面白いのは、ずばりコメントの付け方です。

そもそもC言語でパーサを書くのは、非常に面倒な仕事です。RubyやPythonなどのより高級な言語と比べると、その手間のかかり具合は歴然としています。例えば、メッセージを単語に分割する処理ひとつ取っても、Pythonならmsg.split(" ")と書けば簡潔に記述できるのに対して、C言語だと「文字列の先頭から空白文字を探索し、見つかったらノードを初期化して連結リストに追加して次に進む。見つからなければ...」というように 『手続きの積み重ね』 として表現する必要があります。

これは書くのも面倒なら、読むのも大変なコードです。プログラムの作成者の意図は手続きの積み重ねの中に隠れてしまっているので、他の開発者はコードを地道に追うことでそれを読み取るしかないからです。

Brubeckのコードが斬新なのは、このC言語の技術的な制約をコメントの付け方によって回避している点です。冒頭にコードの一部を引用していますが、Brubeckの開発者はブロックごとに「いまここではプロトコルのこの部分を解析しています」というコメントを付与することで、書き手の意図を読み手に伝える戦略を選びました。

ポイントをはっきりさせるため、コードの一部を取り出して構成を以下に図示します。冒頭のコメントが後続する処理の宣言になっていることが明解に見て取れると思います。

Brubeckのコード解説

私たちは、このコードに感銘を受けたので、自分たちのStatsDパーサでも同じようなブロックコメントを付けることにしました

開発の現場では、時としてコメントは「おまけ」として扱われがちですが、創造的な使い方をすることで、コードの読みやすさを一変させることができるのだ、というのが今回の連載の要点です。

補足: 関連するテクニックについて

このBrubeckの手法に似ているものとして、コミットメッセージの中で記号を使って、変更内容を視覚的に示すというテクニックがあります。

こちらについては、過去記事「わかりやすいコミットメッセージの書き方」 の中で紹介しているので(記事中の「英語だけで表現することにこだわらない」の節を参照)、興味のある方はご一読ください。

2020-01-15

ノータブルコード1 - printfの縦揃え

リーダブルコードの解説の著者の須藤です。リーダブルコードの本編ではプログラマそれぞれがリーダブルなコードを書く方法を紹介しています。リーダブルコードの解説ではチームでリーダブルなコードを書く方法を紹介しています。

では、その次のステップはなんでしょう。それは「チーム外とリーダブルコードの知見を共有する」です。情報を提供するところに情報は集まります。自分たちがリーダブルコードの知見を発信することで知見が集まります。集まるのを待つだけではなく自分から知見を探しにいけばさらに知見を集められます。チーム内だけで知見を共有していると、限られたコンテキストでの知見しか得られなくなってしまいます。チーム外の知見も活用することでチームに新しい知見を導入できます。

たとえば、Ruby on Railsを使ってWebアプリケーションだけを書いている場合はそのコンテキストの知見ばかり集まります。しかし、その知見は、Pythonで機械学習するコードを書いたときに得られる知見とは違います。Pythonで機械学習するコードを書いたときに得られる知見のうちRuby on Railsを使ってWebアプリケーションを書いているときに活かせる知見もあるものです。違うコンテキストの知見を集められればもっとリーダブルなコードを書けるようになります。

実際に自分が違うコンテキストのコードを書かなくても、違うコンテキストで書かれたコードを読むだけでも知見を得られます。(もちろん、自分が書いた方が得られる知見は大きいです。)そのため、クリアコードのメンバーは他の人が書いたコードを読んで学んでいます。野生のリーダブルコードを見つけて学んでいるということです。クリアコードが大事にしているフリーソフトウェアには「プログラムがどのように動作しているか研究し、必要に応じて改造する自由 (第一の自由)」があるので野生のリーダブルコードを学び放題です。

前置きが長くなりましたが、クリアコードのメンバーが普段やっている「他の人が書いたコードから学んだこと」を「ノータブルコード(Notable Code)」としてこのブログで紹介することにしました。前から社内では紹介していたのですが、社内で閉じる必要はないので公開している場所で紹介することにしました。

「ノータブルコード」という名前にした理由は次の通りです。

  • 「リーダブルコード」っぽい!(語感が似ている)
  • 「よい悪い」というより「単に私はここに注目したよ!」というだけだから(「note」ってそういうニュアンスだよね?)
    • 「notable」には「重要な」という「よい」ニュアンスがあるみたいなので本当はアレなんですけど。。。

それでは最初のノータブルコードを紹介します。

Apache ArrowのRのLinux向けパッケージを改良するプルリクエストで見つけたものです。

sprintfを使って文字列をフォーマットしているRのコードです。Rのコードですが、sprintfはいろんな言語にある機能なので、Rを読み書きできない人でも読めるはずです。

https://github.com/apache/arrow/pull/6068/files#diff-3875fa5e75833c426b36487b25892bd8R153-R155

  env_vars <- sprintf(
    "SOURCE_DIR=%s BUILD_DIR=libarrow/build DEST_DIR=%s CMAKE=%s",
    src_dir,                                dst_dir,    cmake
  )

sprintfはフォーマット文字列に実際の値を埋め込んで最終的な文字列を生成しますが、どの値がどこに対応するかわかりにくくなりがちです。このコードでは縦に埋め込み場所と埋め込まれる値を揃えて対応をわかりやすくしています。

他のやり方として、文字列内に式を埋め込む方式(Rubyなら"SOURCE_DIR=#{src_dir}"と書く)や、埋め込み場所に名前を使う方式(Rubyなら"SOURCE_DIR=%{src_dir}" % {src_dir: src_dir}と書ける)がありますが、このスタイルは初めてみました。1行が長すぎると使えないとか、埋め込む値の式が長いと使えないとか使える機会は限定される気がしますがおもしろいなぁと思いました。

おもしろくて思わずコメントしてしまいました。このコードを書いた人によるとリーダブルなコードになるようにいろんな方法を実験していたとのことでした。やっぱり工夫して書いたものだったんですね。

2020年はみなさんも気になったコードを「ノータブルコード」として共有してみませんか?

2020-01-14

タグ:
年・日ごとに見る
2008|05|06|07|08|09|10|11|12|
2009|01|02|03|04|05|06|07|08|09|10|11|12|
2010|01|02|03|04|05|06|07|08|09|10|11|12|
2011|01|02|03|04|05|06|07|08|09|10|11|12|
2012|01|02|03|04|05|06|07|08|09|10|11|12|
2013|01|02|03|04|05|06|07|08|09|10|11|12|
2014|01|02|03|04|05|06|07|08|09|10|11|12|
2015|01|02|03|04|05|06|07|08|09|10|11|12|
2016|01|02|03|04|05|06|07|08|09|10|11|12|
2017|01|02|03|04|05|06|07|08|09|10|11|12|
2018|01|02|03|04|05|06|07|08|09|10|11|12|
2019|01|02|03|04|05|06|07|08|09|10|11|12|
2020|01|02|03|04|05|06|