2016-03-22 7 views
2

Im folgenden Programm nur ein Element, gehe ich data, die viele verschiedene Tage enthält, auf die Funktion GetAvgDayVolm(), die cout Anweisung in dieser Funktion dann 1 ausgibt.Liste mit vielen structs anerkannt als

Aber es sollte mehr als 1 ausgeben, da es mehr als eine unterschiedliche Daten in data gibt. Es sieht aus, curTime wird geändert, obwohl das Programm nicht innerhalb der if Anweisung überhaupt geht. Siehst du etwas falsch?

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

Sie müssen entweder, um die Daten zu kopieren, oder verwenden Sie 'gmtime_r' (' gmtime_s' unter Windows). Letzteres würde ich tun, da es threadsicher ist. – paddy

+0

Nicht verwandt: Entweder du machst eine Menge Einfügungen in der Mitte, Spleiße und ähnliches, oder du solltest die Verwendung von 'std :: list' wahrscheinlich überdenken. –

Antwort

2

Das Problem ist, dass Sie den Zeiger verwenden, der von der gmtime()-Funktion zurückgegeben wird, die im Grunde ein gemeinsamer Teil des Speichers ist, den jeder Aufrufer mit den Zeitfunktionen teilt. Der Weg, das zu beheben, besteht darin, den Wert zu kopieren (durch Dereferenzieren des zurückgegebenen Zeigers), anstatt eine Kopie des Zeigers zu behalten.

In einer Multithread-Umgebung müssten Sie auch weitere Vorkehrungen treffen. Verwenden Sie entweder eine der Thread-sicheren Versionen (gmtime_r/gmtime_s) oder synchronisieren Sie den Zugriff mit einem Mutex (langsam).

Auch würde ich halte Ihre Funktionsparameter von einem Zeiger auf eine konstante Referenz Ändern, weil es immer existieren angenommen wird, und durch die Funktion nicht geändert wird. Hier

ist eine mögliche Lösung für Ihre Funktion:

// 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

Im gmtime manpage, sagt der NOTES Abschnitt:

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

so im Code die CURTIME und movingTime Punkt zum gleichen statischen Datenbereich, sollten Sie gmtime_r stattdessen verwenden, oder einfach nur Speichere das Ergebnis zuerst.