auto キーワードを使って iterator 変数の宣言を簡潔にする

[beru]

C++11 の auto キーワードを使用して変数の型宣言を省きました。
カウンタ変数のスコープを狭めたりもしています。

[m-tmatma]

正規表現で iterator\s で検索したら例えば以下の箇所のようなコードが見つかった。

sakura/sakura_core/cmd/CViewCommander_Cursor.cpp

Line 202 in 27a4524

CMemoryIterator it = GetDocument()->m_cLayoutMgr.CreateCMemoryIterator(pcLayout);

これは iterator というキーワードは含むが、実際には CMemoryIterator という名前の
クラスなので変更対象から除外しているのですか?

[berryzplus]

CMemoryIteratorはイテレータじゃないです。
単にクラスの命名が変わってるだけの、iteratorパターンとは無関係のクラスです。
少なくともこのPRのスコープではなさそう。

[berryzplus]

PRの趣旨は基本的に賛成です。
ただ、個別の変更については「好み」が多分に反映されてくるトピックなので、一括変換ネタではないのかな、と考えています。

たとえば・・・

sakura/sakura_core/CHokanMgr.cpp

Line 181 in 27a4524

for( CPlug::Array::iterator it = plugs.begin(); it != plugs.end(); ++it ){

for( CPlug::Array::iterator it = plugs.begin(); it != plugs.end(); ++it ){
	//インタフェースオブジェクト準備
	CWSHIfObj::List params;
	std::wstring curWord = pszCurWord;
	CComplementIfObj* objComp = new CComplementIfObj( curWord , this, nOption );
	objComp->AddRef();
	params.push_back( objComp );
	//プラグイン呼び出し
	(*it)->Invoke( pcEditView, params );

	objComp->Release();
}

ぼく基準で書き直すとこうなります。

for( auto it = plugs.begin(); it != plugs.end(); ++it ){
	//個々のプラグインを取得
	auto plug = *it;

	//インタフェースオブジェクト準備
	CWSHIfObj::List params;
	auto objComp = new CComplementIfObj( pszCurWord , this, nOption );
	objComp->AddRef();
	params.push_back( objComp );

	//プラグイン呼び出し
	plug->Invoke( pcEditView, params );

	objComp->Release();
}

この修正量を「 auto 対応」で直してしまってよいか疑問に思っています。
しかも、auto利用箇所一個増えとるしｗ

[beru]

@m-tmatma

これは iterator というキーワードは含むが、実際には CMemoryIterator という名前の
クラスなので変更対象から除外しているのですか?

自分がこのPR用にコード変更してた際は、::iterator と ::const_iterator で検索して探してました。
auto 使える箇所は他にもいっぱいあると思いますが、まぁ…。

[beru]

この修正量を「 auto 対応」で直してしまってよいか疑問に思っています。
しかも、auto利用箇所一個増えとるしｗ

自分的にはどちらの書き方でも（一時変数を導入するか）良いと思います。 @berryzplus さんの案に合わせておきますか？

[beru]

std::wstring curWord を削減したのも変更点にあるんですね。まぁこのPRの目的としては auto キーワード使用なのでその変更は入れなくても良いと思いますが、別に入れても問題無いと思います。

[kobake]

イテレータを参照型で受け取るのって一般的です？

[kobake]

素直に auto keyVal = cookies->find(key); でも keyVal の型は自動的に std::map<wstring, wstring>::const_iterator になりますが、それでは不十分と判断されたのでしょうか？

[beru]

確かに素直に auto にしておいた方が良さそうですね。

[kobake]

コメントを入れた「そこで参照型を使う？」以外は概ね良い感じに見えました。

一括変換とはいえこの規模であれば目で追ってレビューできます。

[kobake]

LGTM.

auto による記載の簡潔化に加えて、変数宣言のスコープが広すぎるような悪しき文化もじわじわと駆逐する姿勢が感じられてとても良いです。

[kobake]

マ～ジしちゃいます。他にも話したいことある雰囲気も感じますが、そのへんの続きは別Issueか別PRでやりましょうかね。

[beru]

@kobake レビュー＆マージありがとうございます。
早速ですがこのPRの内容に不具合が見つかりました…。
Issue #293 で報告しました。修正PR #294 も作成しました。
お手数ですが確認お願いします。