1

I have seen this answer (Advantages of using std::make_unique over new operator) where it states:

Don't use make_unique if you need a custom deleter or are adopting a raw pointer from elsewhere.

This is is my code:

void CAutomaticBackupSettingsPage::GetLastBackupDate(COleDateTime& rBackupDate)
{
    DATE* pDatTime = nullptr;
    UINT uSize;

    theApp.GetProfileBinary(_T("Options"), _T("BackupLastBackupDate"), pointer_cast<LPBYTE*>(&pDatTime), &uSize);
    if (uSize == sizeof(DATE))
        rBackupDate = *pDatTime;
    else
        rBackupDate = COleDateTime::GetCurrentTime();

    delete[] pDatTime;
    pDatTime = nullptr;
}

Code analysis gives me two warnings:

enter image description here

and

enter image description here

The latter warning suggests I use std::make_unique but since my pointer data is returned from the GetProfileBinary call, and given the statement in the related question, does that mean I should not use std::make_unique? I admit it is something I have not done before.


The useage of GetProfileBinary clearly states:

GetProfileBinary allocates a buffer and returns its address in *ppData. The caller is responsible for freeing the buffer using delete[].

Barmak Shemirani
  • 30,904
  • 6
  • 40
  • 77
Andrew Truckle
  • 17,769
  • 16
  • 66
  • 164

1 Answers1

1

pDateTime is supposed to be nullptr, and GetProfileBinary handles the allocation. Code Analysis mistakenly thinks you forgot the allocation.

It does need to check for success before calling delete[]. We can't use delete[]pDatTime because pDatTime is not an array. But GetProfileBinary allocates using new BYTE[size], so we need to cast back to BYTE.

You can also add a NULL check before reading pDatTime, that might make Code Analysis happy.

if (pDatTime && uSize == sizeof(DATE))
    rBackupDate = *pDatTime;
else
    rBackupDate = COleDateTime::GetCurrentTime();
if(pDatTime) delete[](BYTE*)pDatTime;

You can use std::unique_ptr<BYTE[]> cleanup((BYTE*)pDatTime) for deletion, but this has to be after GetProfileBinary is called.

Example:

DATE* pDatTime = nullptr;
GetProfileBinary(_T("Options"), _T("BackupLastBackupDate"), (LPBYTE*)(&pDatTime), &uSize);
std::unique_ptr<BYTE[]> cleanup((BYTE*)pDatTime); //automatic delete

if (pDatTime && uSize == sizeof(DATE))
    rBackupDate = *pDatTime;
else
    rBackupDate = COleDateTime::GetCurrentTime();

//pDatTime = NULL; <- Error when used with unique_ptr
...
//pDatTime is deleted later, when `cleanup` goes out of scope
Barmak Shemirani
  • 30,904
  • 6
  • 40
  • 77
  • Thanks @AdrianMole, I think I fixed it, the compile errors anyway. – Barmak Shemirani Oct 03 '21 at 22:52
  • Thanks! Those warnings are now removed if I use both of your tweaks. To prevent the C style cast warning I have used: `std::unique_ptr cleanup(pointer_cast(pDatTime));`. Question 1: Is the final line in my function now obsolete (`pDatTime = nullptr;`) since we are calling this `Cleanup` method? Question 2: In my `GetProfileBinary` call I cast to `LPBYTE*` but in your subsequent code suggestions we use `BYTE*` and `BYTE[]` respectively. I guess this does not matter. – Andrew Truckle Oct 04 '21 at 07:30
  • 1
    1: You don't need `pDatTime = nullptr;` after it has been deleted, because it's a local variable and you are exiting the function. 2: Your call to `GetProfileBinary` is correct, `LPBYTE*` is expected. In a different place, I used `BYTE*`, that relates to how memory is allocated inside `GetProfileBinary`. Also be careful how `std::unique_ptr` is used, the compiler may do weird optimization and break it. If you use it, show it in your question. – Barmak Shemirani Oct 04 '21 at 13:43
  • 2
    @and `cleanup` is not a method. It is an object of type `std::unique_ptr`. It has a non-trivial destructor that runs as the object goes out of scope. It is the destructor that performs the resource cleanup. I'm confused why code analysis doesn't flag that as a *"variable is never used"* issue. – IInspectable Oct 04 '21 at 14:42
  • I have only used `std::unique_ptr` where you suggested ... – Andrew Truckle Oct 04 '21 at 15:10
  • 1
    That's a good point, using `unique_ptr...(pDatTime)` followed by `pDatTime = nullptr` is an error. But it is okay to put `pDatTime = nullptr` after `delete[]...`, in that case it doesn't accomplish anything, but it won't cause an error either. See edit. – Barmak Shemirani Oct 04 '21 at 16:32
  • But it is safe to use my point_cast inside your cleanup call I assume? It is a wrapper for reinterpret_cast. Else I get code analysis warning about c style cast. – Andrew Truckle Oct 04 '21 at 21:18
  • Yes, you can replace that with `reinterpret_cast`. – Barmak Shemirani Oct 04 '21 at 21:24