0

環境はViausl Studio 2015 communityです。

以下のようなファイルと、

    #include "intRect.hpp"

    int main()
    {
      std::cout << "declare..." << std::endl;
      intRect rectangle;

      rectangle.setSize(10, 5);


      std::cout << "set value..." << std::endl;
      // set value
      for (int x = 0; x < 10; x++)
      {
        for (int y = 0; y < 5; y++)
        {
          rectangle.setValue(x, y, x + y);
        }
      }

      std::cout << "printing..." << std::endl;
      // print value
      for (int x = 0; x < 10; x++)
      {
        for (int y = 0; y < 5; y++)
        {
          rectangle.printValue(x, y);
        }
      }
      return 0;

    }

以下のようなインクルードファイルがあるのですが、

    #ifndef INTRECT_HPP
    #define INTRECT_HPP

    #include <memory>
    #include <iostream>

    struct intRect
    {
      int width = 0;
      int height = 0;
      int size = 0;

      //std::unique_ptr<int[]> intrect_body = nullptr;
      std::unique_ptr<int> intrect_body;

      void setSize(const int arg_width, const int arg_height)
      {
        width = arg_width;
        height = arg_height;

        intrect_body.reset(new int(width * height));
      }

      void setValue(const int& arg_x, const int& arg_y, const int& arg_value)
      {
        if (width == 0 || height == 0)
        {
          return;
        }

        int position = arg_y * width + arg_x;

        if (position > width * height)
        {
          return;
        }

        intrect_body.get()[position] = arg_value;
      }

      void printValue(const int& arg_x, const int& arg_y)
      {

        if (width == 0 || height == 0)
        {
          return;
        }

        int position = arg_y * width + arg_x;


        if (position > width * height)
        {
          return;
        }

        std::cout << "value at position (" << arg_x << ", " << arg_y << ") is " << intrect_body.get()[position] << std::endl;

      }


    };


    #endif 

以下の画像のように、止まるところがおかしなところで、勝手にリソースが解放されてしまっているようにも見えるのですが、原因がわかりません。
アウトプットには
Exception thrown at 0x0F6FB4E4 (ucrtbased.dll) in smapowraptest.exe: 0xC0000005: Access violation reading location 0x000003B2.
と表示されています。
Exception thrown at 0x775A324F (ntdll.dll) in smapowraptest.exe: 0xC0000005: Access violation reading location 0x00000009.
と表示されることもあり、挙動がよくわからない状態になります。

MSYS2でg++でコンパイルして流すと、正常に動くのですが、何か間違っているのでしょうか。

画像の説明をここに入力

2
  • 3
    int型の配列を確保するときは、new int[width * height]と書きます。new int(width * height)と書くとint型ひとつ分の領域を確保してwidth * heightの値で初期化します。g++で動くのは偶然です。
    – h2so5
    Commented 2016年1月23日 13:51
  • ありがとうございます。解決しました。
    – Nick
    Commented 2016年1月23日 13:58

2 件の回答 2

6

std::unique_ptr<int>intへのポインターを保持するものです。int[]配列を保持するのであればコメントアウトされているようにstd::unique_ptr<int[]>を使用する必要があります。

またnew int(width * height)はintを格納するメモリを確保した上でintの値をwidth * heightで初期化します。new int[width * height]とすべきですが、それ以上に、(スマートポインターを使用している以上、自動的に解放されるものの)ソースコード上の対称性としてnewしておいてdeleteしないというのも不安にかられます。これについてはnewによる確保とコンストラクターの呼び出し、その上でstd::unique_ptrの構築までを一気に済ませてくれるstd::make_unique()が用意されています。

setSize()する前にsetValue() printValue()を呼ばれると困るため事前チェックをされているようですが、であればsetSize()はコンストラクターとすべきです。コンストラクターであればほぼ確実に呼ばれますからチェックも必要なくなります。

事前チェック自体も、異常状態・不正値を無視してreturnしてしまうのも無責任です。何らかのassertを行うことをお勧めします。Visual C++ではデバッグビルドではエラーレポートしリリースビルドではコードが削除される_ASSERTEマクロが用意されています。

以上を総合すると

struct intRect {
    int width;
    int height;
    std::unique_ptr<int[]> intrect_body;

    intRect(int arg_width, int arg_height) : width(arg_width), height(arg_height), intrect_body(std::make_unique<int[]>(width * height)) {}

    void setValue(int arg_x, int arg_y, const int& arg_value) {
        _ASSERTE(arg_x < width && arg_y < height);
        intrect_body[arg_y * width + arg_x] = arg_value;
    }

    void printValue(const int& arg_x, const int& arg_y) {
        _ASSERTE(arg_x < width && arg_y < height);
        std::cout << "value at position (" << arg_x << ", " << arg_y << ") is " << intrect_body[arg_y * width + arg_x] << std::endl;
    }
};

と書けます。

3
  • すばらしいコードレビューをありがとうございます。理解が深まりかつ勉強になり、感謝します。ご教示いただいたコードで、動作確認できました。
    – Nick
    Commented 2016年1月23日 14:24
  • @sayuri std::unique_ptrに設定されたポインタは自動でdeleteされます。std::make_unique()を使うかどうかは関係ありません。
    – h2so5
    Commented 2016年1月23日 15:00
  • @h2so5 スマートポインターを使用している以上、自動でdeleteされることは前提として端折ってしまいました。加筆します。
    – sayuri
    Commented 2016年1月23日 21:07
0

MSYS2でg++でコンパイルして流すと、正常に動くのですが、何か間違っているのでしょうか。
g++でコンパイルして動くのはメモリ割当ルールの違いなどのせいで「たまたま」動いているように見えるだけだと思われます。最近C++のコードは書いていないのですが、そういう人間から見ても、あなたのコードはかなり無茶苦茶ですね。

std::unique_ptr<int[]> intrect_body = nullptr;

複数のint型の値を保持しないといけないのに、なぜ配列の方をコメントアウトしてしまっているのでしょう。[]付きが正解です。

    intrect_body.reset(new int(width * height));

コメントにもありますが、これはintを1つ分確保して、その値をwidth * heightにしているだけです。width * height個分のintを確保するなら、

    intrect_body.reset(new int[width * height]);

と書かないといけません。

    intrect_body.get()[position] = arg_value;

これは趣味の問題で、絶対ダメというわけではないのですが、unique_ptrのoperator[]はうまく定義されていて、要素をアクセスするだけなら、get()は要りません。

    intrect_body[position] = arg_value;

(もう一箇所のget()も同様。)

上記を全部直してもまだ何かしらエラーが出るのならコメント等でご連絡ください。

この質問に回答するには、ログインする必要があります。

求めていた回答ではありませんか? のタグが付いた他の質問を参照する。