1

I'm trying to build a timer in C++. In my Timer class there are two Date objects that hold a std::unique_ptr<struct tm> pointer. After I std::move the second unique_ptr in the second Date object in the following piece of code, it points to the same memory address of the first one, effectively making the two objects represent the same time, even though they should be different because of the duration offset.

    using namespace std;
    time_t localTime = time(nullptr);
    unique_ptr<struct tm> currentTime = static_cast<unique_ptr<struct tm>>(localtime(&localTime));
    startDate = Date(std::move(currentTime));

    time_t endTime = time(nullptr) + duration;
    unique_ptr<struct tm> timerEndTime = static_cast<unique_ptr<struct tm>>(localtime(&endTime));
    endDate = Date(std::move(timerEndTime));

The Date constructor being called is this:

Date::Date(std::unique_ptr<struct tm> time) : date(std::move(time)) {}

What am I doing wrong?

loremol
  • 25
  • 4
  • You use `static_cast>` but smart pointers do not work like that. use `std::make_unique(localtime(&localTIme))`. Also the notation `struct tm` seems to indicate you're more used to writing "C" code, but `struct` is not needed here. Follow this link [std::make_unique](https://en.cppreference.com/w/cpp/memory/unique_ptr/make_unique) for more information on std::make_unique – Pepijn Kramer Jun 15 '23 at 09:28
  • @DanielLangr good question. It had better not be `Data(tm* time)` – Pepijn Kramer Jun 15 '23 at 09:37
  • Pepijn, I tried to use `make_unique` but it creates an error with `tm`'s constructor. By the way, thank you for the `struct` syntax advice. – loremol Jun 15 '23 at 09:41
  • 1
    According to cppreference, `localtime` returns a (raw) _"pointer to a static internal `std::tm` object on success"_. Why do you cast it to `std::unique_ptr`? It does not make any sense (moreover, AFAIK, it's UB). A unique pointer needs to logically own its pointee (at least with a default deleter). – Daniel Langr Jun 15 '23 at 09:45
  • Me bad I should have checked the documentation on tm. But casting to a unique_ptr is at least not something you should do. Regardless of the propeties of `std::tm` Are you trying to get rid of raw pointers in your code (coding guidelines)? – Pepijn Kramer Jun 15 '23 at 09:47
  • @DanielLangr I used unique ptr's just to experiment with them. Should've read more about them and `` before. Thank you all for your kind help – loremol Jun 15 '23 at 09:52
  • @loremol Learning C++ by experimenting is not a good idea. You will frequently end up with undefined behavior, which is extremely tricky, since the observed behavior may match that expected one. – Daniel Langr Jun 15 '23 at 10:23
  • @DanielLangr You're right. – loremol Jun 15 '23 at 10:58

1 Answers1

4

It is unsurprising that the two objects overlap, because that is how std::localtime is supposed to work:

std::tm* localtime( const std::time_t *time );

Return value

pointer to a static internal std::tm object on success, or null pointer otherwise.

- See std::localtime on cppreference

If you create a std::unique_ptr that wraps the results of std::localtime, you will attempt to free an object with delete which was never allocated with new, and that is undefined behavior. Write:

std::time_t localTime = std::time(nullptr);
std::tm* currentTime = std::localtime(&localTime);
startDate = Date(*currentTime);

std::time_t endTime = std::time(nullptr) + duration;
std::tm* timerEndTime = std::localtime(&endTime);
endDate = Date(*timerEndTime);

Your Date constructor should be receiving a std::tm or std::tm const& and store it by value. If you stored a pointer, then all Dates would contain the same pointer to a static internal object.

How do I prevent unique pointers from overlapping in general?

This is easy to do, because std::unique_ptr is more or less fool-proof. As long as you:

  • immediately take ownership of pointers to new memory, or
  • exclusively use std::make_unique, and
  • never call std::unique_ptr::release()

... you won't be able to accidentally make two smart pointers overlap.

Jan Schultke
  • 17,446
  • 6
  • 47
  • 96