2016-03-22 5 views
2

次のプログラムでは、多くの異なる日数を含むdataを関数GetAvgDayVolm()に渡し、この関数内のcoutステートメントは1を出力します。多くの構造体が1つのメンバとして認識されているリスト

dataに複数の異なる日付があるため、出力は1以上である必要があります。プログラムはifの中に入っていませんが、curTimeのように見えます。あなたは間違ったことを見ますか? gmtimeはマンページで

long int GetAvgDayVolm(list<struct DataPoint>* data) 
{ 
    long long int totalVolm = 0; 
    long int numOfDays = 1; 
    struct DataPoint dp = (*data).front(); 
    time_t rawTime2 = dp.timeStamp; 
    time_t rawTime = 0; 
    struct tm* curTime = gmtime(&rawTime2); 

    struct tm* movingTime = new struct tm(); 

    for(list<struct DataPoint>::iterator it = (*data).begin(); it != (*data).end(); ++it) 
    { 
     rawTime = (*it).timeStamp; 
     movingTime = gmtime(&rawTime); 
     totalVolm += (*it).volm; 

     if(curTime->tm_mday != movingTime->tm_mday || 
      curTime->tm_mon != movingTime->tm_mon || 
      curTime->tm_year != movingTime->tm_year) 
     { 
      numOfDays = numOfDays + 1; 
      curTime = movingTime; 
     } 
    } 

    cout<<numOfDays<<endl; 
    return 0; 
} 
+1

あなたはデータをコピー・アウト、または 'gmtime_r'(Windows上で' gmtime_s')を使用するのいずれかが必要です。後者はスレッドセーフであるため、私はこれを行います。 – paddy

+0

関連していません:真ん中やスプライスなどで挿入が非常に多いか、おそらく 'std :: list'の使用法を再考するべきです。 –

答えて

2

問題は、gmtime()関数から返されたポインタを使用していることです。これは基本的には、時間関数を使用するすべての呼び出し元が共有するメモリの共通チャンクです。これを修正する方法は、ポインタのコピーを保持するのではなく、(返されたポインタの逆参照によって)値をコピーすることです。

マルチスレッド環境では、さらに注意する必要があります。スレッドセーフバージョン(gmtime_r/gmtime_s)のいずれかを使用するか、またはミューテックス(遅い)を使用してアクセスを同期してください。常に存在すると仮定され、関数によって変更されていないため、

また、私はconst参照ポインタからあなたの関数のパラメータを変更することを検討します。ここで

は、あなたの機能のための可能な解決策である:

// pass by const reference if possible because 
// the list is assumed to exist and is never modified 
long int GetAvgDayVolm(const list<DataPoint>& data) 
{ 
    if(data.empty()) // possible crash later without this check 
     return 0; 

    long long int totalVolm = 0; 
    long int numOfDays = 1; 
    DataPoint dp = data.front(); // REQUIRES list contains at least one element 
    time_t rawTime2 = dp.timeStamp; 
    time_t rawTime = 0; 

    // don't use pointer here, dereference the 
    // returned value 
    std::tm curTime = *gmtime(&rawTime2); 

    // no need for pointer here - dereference the 
    // return value of gmtime() instead 
    std::tm movingTime; 

    for(list<DataPoint>::const_iterator it = data.begin(); it != data.end(); ++it) 
    { 
     rawTime = it->timeStamp; 
     movingTime = *gmtime(&rawTime); 
     totalVolm += it->volm; 

     if(curTime.tm_mday != movingTime.tm_mday || 
      curTime.tm_mon != movingTime.tm_mon || 
      curTime.tm_year != movingTime.tm_year) 
     { 
      numOfDays = numOfDays + 1; 
      curTime = movingTime; 
     } 
    } 

    cout<<numOfDays<<endl; 
    return 0; // is this correct?? 
} 
3

NOTESセクションが言う、:あなたのコードで非常に

The four functions asctime(), ctime(), gmtime() and localtime() return a pointer to static data and hence are not thread-safe 

CURTIMEとmovingTimeポイント同じ静的データ領域に、あなたの代わりにgmtime_r使うべきか、単に最初に結果を保存します。