質問をすることでしか得られない、回答やアドバイスがある。

15分調べてもわからないことは、質問しよう!

ただいまの
回答率

89.10%

以下のrange-based forは危険でしょうか。

解決済

回答 3

投稿 編集

  • 評価
  • クリップ 2
  • VIEW 853

__ook

score 39

以下の記事を読んでいたところ
https://qiita.com/rinse_/items/ad0cc7e351e836595c94

// 危険なコード
for( auto e : something().get_vector() ) // get_vectorは内部に持つvectorへの参照を返す
{
  // e is destroyed
}

// 安全なコード
for( auto e : std::vector < int > { 1,2,3,4,5,6,7,8,9,0 } )
{
  // e is 1,2,3,...,0
}

range-based forでは、コンテナへの参照をキャプチャします。上の危険な例ではキャプチャしたものがすぐに破棄されてしまいますが、下の安全な例では、一時オブジェクトをキャプチャして延命しているので安全に利用することができるのです。

という文言があったため、試してみたところ、正しく動いているように見えました。
これはなぜ動いてしまうのでしょう。
環境はVisual Studio 2015です。

また、

#include <iostream>    
#include <fstream>
#include <string>
#include <vector>

using namespace std;

auto retVec(vector<string> original)
{
    return original;
}

struct str
{
    vector<string> _vec
    {
        "first",
        "second",
        "third"
    };
public:
    auto get_vec() { return _vec; };
};

int main()
{
    for (auto e : str().get_vec())    // 危険?
    {
        cout << e << endl;
    }

    str str_exist;
    for (auto e : str_exist.get_vec())    // これも危険?
    {
        cout << e << endl;
    }
}

2つ目のrange-based forは危険でしょうか?

【追記】

#include <iostream>    
#include <fstream>
#include <string>
#include <vector>

using namespace std;


struct str
{
    vector<int> _vec
    {
        0,
        1,
        2
    };
public:
    auto& get_vec() { return _vec; }
};

int main()
{
    for (auto e : str().get_vec())
    {
        cout << e << endl;
    }
}


上記なら危険なのですね。

  • 気になる質問をクリップする

    クリップした質問は、後からいつでもマイページで確認できます。

    またクリップした質問に回答があった際、通知やメールを受け取ることができます。

    クリップを取り消します

  • 良い質問の評価を上げる

    以下のような質問は評価を上げましょう

    • 質問内容が明確
    • 自分も答えを知りたい
    • 質問者以外のユーザにも役立つ

    評価が高い質問は、TOPページの「注目」タブのフィードに表示されやすくなります。

    質問の評価を上げたことを取り消します

  • 評価を下げられる数の上限に達しました

    評価を下げることができません

    • 1日5回まで評価を下げられます
    • 1日に1ユーザに対して2回まで評価を下げられます

    質問の評価を下げる

    teratailでは下記のような質問を「具体的に困っていることがない質問」、「サイトポリシーに違反する質問」と定義し、推奨していません。

    • プログラミングに関係のない質問
    • やってほしいことだけを記載した丸投げの質問
    • 問題・課題が含まれていない質問
    • 意図的に内容が抹消された質問
    • 過去に投稿した質問と同じ内容の質問
    • 広告と受け取られるような投稿

    評価が下がると、TOPページの「アクティブ」「注目」タブのフィードに表示されにくくなります。

    質問の評価を下げたことを取り消します

    この機能は開放されていません

    評価を下げる条件を満たしてません

    評価を下げる理由を選択してください

    詳細な説明はこちら

    上記に当てはまらず、質問内容が明確になっていない質問には「情報の追加・修正依頼」機能からコメントをしてください。

    質問の評価を下げる機能の利用条件

    この機能を利用するためには、以下の事項を行う必要があります。

質問への追記・修正、ベストアンサー選択の依頼

  • t_obara

    2019/12/17 10:25

    とりあえず、参照されているページと同様、デストラクタを印字して確かめてみてはいかがですか?

    キャンセル

回答 3

checkベストアンサー

+1

前提

まずRange-based forがどのように展開されるか確認しましょう。

§9.5.4 [stmt.ranged]/1
The range-based for statement

for ( for-range-declaration : for-range-initializer ) statement

is equivalent to

{
    auto &&__range = for-range-initializer ;
    auto __begin = begin-expr ;
    auto __end = end-expr ;
    for ( ; __begin != __end; ++__begin ) {
        for-range-declaration = *__begin;
        statement
    }
}

そしてそれぞれ見ていきましょう。

1つ目の例

for( auto e : something().get_vector() ) // get_vectorは内部に持つvectorへの参照を返す
{
  // e is destroyed
}

危険です。__range の型はstd::vector<int>&になります。このときsomething()の寿命は尽きています。つまり存在しないオブジェクトを束縛しています。

https://wandbox.org/permlink/v0c1cR7NzOfaPydd

Linux上でAddressSanitizerに掛けると

./a.out 
destructor
=================================================================
==5022==ERROR: AddressSanitizer: stack-use-after-scope on address 0x7ffd5b3fe1e0 at pc 0x0000004c70dd bp 0x7ffd5b3fe070 sp 0x7ffd5b3fe068
READ of size 8 at 0x7ffd5b3fe1e0 thread T0
    #0 0x4c70dc in __gnu_cxx::__normal_iterator<int*, std::vector<int, std::allocator<int> > >::__normal_iterator(int* const&) /usr/bin/../lib/gcc/x86_64-linux-gnu/7.4.0/../../../../include/c++/7.4.0/bits/stl_iterator.h:783:20
    #1 0x4c5eed in std::vector<int, std::allocator<int> >::begin() /usr/bin/../lib/gcc/x86_64-linux-gnu/7.4.0/../../../../include/c++/7.4.0/bits/stl_vector.h:564:16
    #2 0x4c57f2 in main /home/yumetodo/teratail/229984/example1/main.cpp:17:15
    #3 0x7f69dc193b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)
    #4 0x41b659 in _start (/home/yumetodo/teratail/229984/example1/a.out+0x41b659)

Address 0x7ffd5b3fe1e0 is located in stack of thread T0 at offset 32 in frame
    #0 0x4c558f in main /home/yumetodo/teratail/229984/example1/main.cpp:15

  This frame has 5 object(s):
    [32, 56) 'ref.tmp' (line 17) <== Memory access at offset 32 is inside this variable
    [96, 112) 'ref.tmp1' (line 17)
    [128, 168) 'ref.tmp2' (line 17)
    [208, 216) '__begin1' (line 17)
    [240, 248) '__end1' (line 17)
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-use-after-scope /usr/bin/../lib/gcc/x86_64-linux-gnu/7.4.0/../../../../include/c++/7.4.0/bits/stl_iterator.h:783:20 in __gnu_cxx::__normal_iterator<int*, std::vector<int, std::allocator<int> > >::__normal_iterator(int* const&)
Shadow bytes around the buggy address:
  0x10002b677be0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10002b677bf0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10002b677c00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10002b677c10: 00 00 00 00 00 00 00 00 f1 f1 f1 f1 00 f3 f3 f3
  0x10002b677c20: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x10002b677c30: 00 00 00 00 00 00 00 00 f1 f1 f1 f1[f8]f8 f8 f2
  0x10002b677c40: f2 f2 f2 f2 f8 f8 f2 f2 f8 f8 f8 f8 f8 f2 f2 f2
  0x10002b677c50: f2 f2 00 f2 f2 f2 f8 f3 f3 f3 f3 f3 00 00 00 00
  0x10002b677c60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10002b677c70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10002b677c80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==5022==ABORTING

のように言われます。(MicrosoftがMSVCに試験実装したASanは何も言ってこない、Releaseビルドでしか使えないから最適化でUBだから吹き飛んでるので見つからないんじゃないかと推測)

ちなみにメンバー関数の宣言時にref qualifierをつけるとコンパイル時にこのようなことを防げます。

std::vector < int >& get_vector() & { return ( _Value ); }
prog.cc: In function 'int main()':
prog.cc:17:62: error: passing 'something' as 'this' argument discards qualifiers [-fpermissive]
   17 |   for( auto e : something { 1,2,3,4,5,6,7,8,9,0 }.get_vector() ) // get_vectorは内部に持つvectorへの参照を返す
      |                                                              ^
prog.cc:10:24: note:   in call to 'std::vector<int>& something::get_vector() &'
   10 |   std::vector < int >& get_vector() & { return ( _Value ); }
      |                        ^~~~~~~~~~

https://wandbox.org/permlink/eRayizx8KL07dgja

2つ目の例

  for( auto e : std::vector < int > { 1,2,3,4,5,6,7,8,9,0 } )
  { // 一時オブジェクトはキャプチャされ延命されている
      std::cout << e;
  }

安全です。__range の型はstd::vector<int>&&になります。このとき一時オブジェクトはrvalue referenceによって束縛されているので寿命が延長されます。

https://wandbox.org/permlink/C6vnwrPNcDHazhot

3つ目の例

    for (auto e : str().get_vec())    // 危険?
    {
        cout << e << endl;
    }

安全です。ここでget_vecの戻り値の型がstd::vector<std::string>であることに注意してください。つまりこの関数はメンバー変数の参照ではなくてコピーを返しています。したがって2つめの例と同様のことが起こります。

https://wandbox.org/permlink/cUE16XoExbGq6X1T

4つ目の例

    str str_exist;
    for (auto e : str_exist.get_vec())    // これも危険?
    {
        cout << e << endl;
    }

安全です。3つ目と同様なので割愛します。

投稿

編集

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

  • 2019/12/18 15:38

    「return文での"通常ローカル変数に対する"std:moveは常に間違ってる」なら妥当とは思います。
    暗黙のムーブになる条件やNRVO(copy elision)阻害条件はそれなりに複雑ですね。
    https://timsong-cpp.github.io/cppwp/n4659/class.copy.elision

    キャンセル

  • 2019/12/19 11:11

    回答ありがとうございます…
    なるほど、僕が真似た(つもりだった)コードはそもそもコピーを返しているから安全なのですね、読み違えていました。

    キャンセル

  • 2019/12/19 13:55

    まあそういうわけで、
    https://teratail.com/questions/228746
    での回答は一部撤回ですね。

    キャンセル

+1

こんにちは。

私もあれ?っと思ったのですが、どうも範囲ベースforのコンテナへの参照は右辺値参照で参照されるからのようです。
この旨のコードが範囲for文に記載されていました。

従って、ここで回答したのと同じ原理で破棄されてしまうということのようです。

wandboxで実験のコードの前者は上記質問のNG1と同じ原理です。

2つ目のrange-based forは危険でしょうか?

str_existの寿命はmain関数の終わりまであるので、危険ではないです。
これに対して、for文の「コンテナを指定する式の一時オブジェクト」の寿命はその式が終わるまでですので、for文のループ・ブロックに入る前に破棄されます。

投稿

編集

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

  • 2019/12/19 11:16

    ああ…なるほど…ありがとうございます。
    つまり…参照で返却した場合iteratorで回すのが正、でしょうか?

    キャンセル

  • 2019/12/19 11:38

    iteratorだろうが範囲ベースだろうが、ループ対象のコンテナ・オブジェクトがfor文の終わりまで生きているようなコードならOKです。
    iteratorで、beginとendのオブジェクトについて、それぞれ異なる一時オブジェクトを指定する人はいないとは思いますが、油断するとやるかもですね。例えば下記はダメです。
    for( auto it = something().get_vector().begin(); it != something().get_vector().end(); ++it)
    {
    }

    キャンセル

  • 2019/12/19 13:15

    やったことがあるやつでした…
    ありがとうございます。

    キャンセル

+1

結論からいえば、質問文中コードの場合は両range-based forともに「安全」です。

struct str
{
   // (略)
   auto get_vec() { return _vec; };
};

strクラス(構造体)get_vecメンバ関数が、データメンバ_vecへの参照ではなくコピー結果を一時オブジェクトとして返すインタフェースのため、参考にされたQiita解説記事とは異なる結果になっています。


もし、get_vecメンバ関数がデータメンバへの参照を返すインタフェース(例えば戻り値型がauto&)だとすると:

  • 前者str().get_vec()は「危険」です。一時オブジェクトstr()とともに参照先vector<string>実体も破棄されてしまうため、走査中のイテレータは無効オブジェクトを指してしまいます。
  • 後者str_exist.get_vec()は「安全」です。変数str_existの生存期間中は参照先も生存していますから、安全にvector<string>実体へとアクセスできます。

余談:個人の感性にも左右されますが、get_vecというメンバ関数名からはデータメンバへの参照を返すインタフェースを想起させますし、効率的なC++クラス設計としても参照を返す方がパフォーマンス上好ましいです。本当に「コピーされたデータを返す」のであれば、clonecopyなどの名称を含めたほうがよいかもしれません。

投稿

編集

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

  • 2019/12/19 11:13

    すごく理解できました。ありがとうございます。
    > 余談:個人の感性にも左右されますが、get_vecというメンバ関数名からはデータメンバへの参照を返すインタフェースを想起させますし、効率的なC++クラス設計としても参照を返す方がパフォーマンス上好ましいです。
    なるほど…参照を返却すべきなのですね、ただその場合はrange-based forを使用することは望ましくない、と…

    キャンセル

  • 2019/12/20 10:57

    これはクラス設計の問題ですから、明確な正解は存在しないと思います。
    yumetodoさん回答のコメント部に書いたように、例えば https://wandbox.org/permlink/7fFSaBCQm81kOKsP という解法もありえます。
    上記では参照返却による効率性と、ムーブされたオブジェクト返却による安全性を両立させたものになっています。(代償として、より高度なC++の知識を要求することにはなりますが...)

    キャンセル

  • 2019/12/20 11:26

    なるほど…たしかにこれなら安全に一時オブジェクトを参照できますね…

    キャンセル

15分調べてもわからないことは、teratailで質問しよう!

  • ただいまの回答率 89.10%
  • 質問をまとめることで、思考を整理して素早く解決
  • テンプレート機能で、簡単に質問をまとめられる