現在のプロジェクトのソースコードを静的解析ツールにかけた結果がレポートされたので、昨日はその内容を見ていました。
ツールが警告を出した場合、警告が出た箇所にどのように対処するかを報告しないといけないのですが、その報告の中に「現状とする／意図どおり」というのがあり、気になったのでそのコードを見てみると。
int sample_code_A(int n) { if(unsigned(n) > 255) { n = n < 0 ? 0 : 255; } return n; }
（関数名とか元のコードから一部変えてあります）
ちょっと考えて。引数の値が0未満なら0を255より大きければ255をそうでなければ引数の値をそのままの返す関数だと理解。
意図どおりかもしれないけれど、うまいコードとも言えず。うまいか否かはおいておくとしても、とてもよいコードとは言えず。
プログラムは意図どおりに動くようにコードを書くのはもちろんですが、加えて読み手に意図が伝わるように書かなければいけないわけで。
正直、これで充分でしょう。
int sample_code_B(const int n) { if (n < 0) return 0; else if(n < 255) return n; else return 255; }
あるいは三項演算子を使って。
int sample_code_C(int n) { return n < 0 ? 0 : n < 255 ? n : 255; }
こころみに。これをコンパイルするとどういうアセンブリコードになるかも確かめてみました。
Mac OS X 10.5 (PowerPC)上でg++でg++-4.2 -ansi -Wall -arch x86_64 -O2 -Sというオプションでそれぞれコンパイルしてみました*1。
結果。
__Z13sample_code_Ai:
pushq %rbp
movq %rsp, %rbp
cmpl $255, %edi
jbe L8
sarl $31, %edi
notl %edi
andl $255, %edi
L8:
movl %edi, %eax
leave
ret
__Z13sample_code_Bi:
pushq %rbp
movq %rsp, %rbp
testl %edi, %edi
js L20
cmpl $255, %edi
movl $255, %eax
cmovge %eax, %edi
L17:
movl %edi, %eax
leave
ret
L20:
xorl %edi, %edi
jmp L17
__Z13sample_code_Ci:
pushq %rbp
movq %rsp, %rbp
movl $255, %edx
cmpl $255, %edi
cmovle %edi, %edx
xorl %eax, %eax
testl %edx, %edx
cmovns %edx, %eax
leave
ret
わたしのアセンブラの知識はせいぜいi386どまりなので、きちんとした判断は下せませんが、このサイズですしどれも同じようなものでしょう。ジャンプがない分sample_code_Cが多少効率的かもしれません。それでも些細な差だと思いますが。
効率的だ、とか、うまいこと書いた、とか思っても、実はそうでもないわけで。それよりも可読性が落ちる分、最初のコードはトータルでは「非効率なコード」でしょう。
意図どおりに動くのは必達。でもそれだけでは半分。読んでわかるようにすることも重要。とても重要。
…という思いをあらためて感じた一件。
余談。
どうせやるならこれぐらいやってみようぜ、と書いたコード。
int sample_code_D(int n) { static const int BitsPerChar = 8; return (n & ~255) ? ((~n >> (sizeof(int) * BitsPerChar - 1)) & 0xff) : n; }
上と同じ条件でコンパイルしたらsample_code_Cより長くなった…orz。
__Z13sample_code_Di:
pushq %rbp
movq %rsp, %rbp
testl $-256, %edi
je L24
movl %edi, %eax
notl %eax
sarl $31, %eax
movzbl %al,%edi
L24:
movl %edi, %eax
leave
ret
