Calculator: Update Calculator engine’s use of smart pointers

[MicrosoftIssueBot]

The Calculator engine allocates and frees its own memory which leads to issues such as double-frees that are hard to investigate. We should clean this code and switch to modern constructs such as smart_ptrs to eliminate these issues.

[MicrosoftIssueBot]

This is your friendly Microsoft Issue Bot. I created this issue automatically as requested by a team member.

[mcooley]

We've made some progress here (see, for example, #12) but there's still some code which calls functions like destroyrat and NumObjDestroy which we should try to refactor away.

[vlogozzo]

Hello, i'm new to the project and would like to work on this refactor, I looked through Rational.cpp and saw lots of places where smart pointers could be useful, any other places I should check before I look for more?

[danbelcher-MSFT]

The engine is C code compiled with a C++ compiler. We've been making updates over time to improve the safety and practices of the codebase. For this issue, the goal is to eliminate use of naked new operators and move to smart pointers. This applies throughout the solution. In general, looking for uses of 'new' and raw pointers, then finding ways to eliminate them or make them safe, is the best way to resolve this issue.

[vlogozzo]

I've found lots of ref new and zmalloc which from what I can find handle there own memory, is the plan to replace these with smart pointers? or are there naked new operators still in the code?

[danbelcher-MSFT]

ref new should stay for now. In CX, these are "hat" pointers; essentially, they are smart pointer types. zmalloc can be removed.

Here is an instance of naked new that can be removed:
https://github.com/Microsoft/calculator/blob/21e15c426e35083f5f6d203b86cea597a47bcb57/src/CalcViewModel/StandardCalculatorViewModel.cpp#L512

Because the array is constant size, we can fix this by allocating the array on the stack:

wchar_t temp[100];

[vlogozzo]

Im trying to run the Unit test, is it intended to open as a blank program? or is it supposed to give me some feedback img

[danbelcher-MSFT]

Yep, a blank app for the unit tests is intentional. The UTs cover classes like the ViewModels which can only be instantiated in a UWP app.

[vlogozzo]

the problem I keep running into is the .ToPRAT() can be used with smart pointers sent the type of RAT, but every function in the engine is expecting a PRAT*, so a conversion of that would need to be made or the entire engine would need to be refactored(if thats the point of the issue, i can attempt to refactor, but at that point you could rewrite the engine?)

[vlogozzo]

excuse my variable naming, ill clean them up in a pull, but something like this is currently working

Rational RationalMath::Pow(Rational const& base, Rational const& pow)
{
    //PRAT baseRat = base.ToPRAT();
    PRAT powRat = pow.ToPRAT();
    shared_ptr<RAT> test(base.ToPRAT());
    auto PRAT = test.get();
    try
    {
        powrat(&PRAT, powRat, RATIONAL_BASE, RATIONAL_PRECISION);
        destroyrat(powRat);
    }
    catch (DWORD error)
    {
        //destroyrat(baseRat);
        destroyrat(powRat);
        throw(error);
    }

    Rational result{ test.get() };
    //destroyrat(baseRat);

    return result;
}

[vlogozzo]

@danbelcher-MSFT thank you for the feed back in the PR, I noticed that this wasn't a elegant solution. I had a question about the solution you mentioned, you mentioned making ToPRAT return a smart-pointer, would you keep the ratpack functions the same? or change them all? I was looking into changing more things in RationalMath but all the ratpack functions preform swaps on the pointers, which trips up the smart-pointer.

[danbelcher-MSFT]

In the PR, I mentioned that we wanted to eliminate PRAT entirely, thereby solving this issue because there would be no manual allocations of RAT structs. So, perhaps we can simply close this issue and write a new issue with a roadmap for how we eliminate PRAT. I don't know that it's a good use of your time to go update the codebase so that PRATs are managed with smart pointers if our goal is to eventually remove these types entirely.

To answer your question specifically, if we _were_ to update uses of PRAT to instead use smart pointers where appropriate, we can now use unique_ptr<RAT> as a way to express ownership of a dynamically allocated RAT. A side-effect of this would be that a naked RAT pointer (aka, PRAT) would imply _non-ownership_. Then we could update ratpak as follows:

1. Use unique_ptr<RAT> when ownership of memory needs to be expressed.
2. Where possible, update ratpak functions to accept const RAT& or RAT&. References are guaranteed to refer to an object by the language standard, so it's a tighter contract then passing a pointer that may be null. The ability to make this change is dependent on the function's implementation. If the function assumes that the parameter PRAT is not null, then it can be updated to instead accept a RAT&.
3. Leave the remaining ratpak functions as accepting a PRAT and, given a unique_ptr<RAT> uniqueRat;, call ratpack functions passing uniqueRat.get().

If there are problems where the ratpack functions are swapping pointers, we should update those functions to work with the smart pointer paradigms.

[vlogozzo]

with the planned removal of create/delete rat/num where should allocation of the structs be handled

[danbelcher-MSFT]

They likely can be replaced with calls to make_unique. For example:

PRAT tmp;
createrat(tmp);

can become:

auto tmp = make_unique<RAT>();

[vlogozzo]

so im making progress I have the Rational's operator+= and all the functions it calls switched over to smart pointer references, but now upon leaving scope im getting a memory leak from MANTTYPE mant[], and both c and c++ style deallocations wont work in a destructor, any advice or insight on this varriable.

[danbelcher-MSFT]

Yes, memory is leaking because delete[] was not called on the data member. As discussed in #211, using arrays this way is non-standard behavior and we should change the member to std::vector<MANTTYPE> mant;. I think @fwcd is already working on that change.

[vlogozzo]

if its worth anything delete[] wont compile when used on MANTTYPE, but ill look out for the rewrite and work from there

[danbelcher-MSFT]

Yeah, delete[] was a bad suggestion on my part. The memory isn't dynamically allocated.

[vlogozzo]

@danbelcher-MSFT I've noticed the conversaion on #211 has moved towards putting the NUMBER allocation onto the stack, making this issue non-existent. Is this the plan or should I continue with this bug? Also is there any related bugs in the project currently that I can look into?

[danbelcher-MSFT]

Stack-allocation seems like the best choice here so it's likely that change will be brought in to master. We get better performance and memory safety. I said earlier that this issue should probably be closed and instead we should have an issue focusing on how to eliminate RAT and NUM structs. The gist is that these structs are remnants from when the codebase was pure C code. We've created Rational and Number classes that support intuitive functionality using operator overloads. Internally, these classes must still convert back to RAT and NUM structs in-order to perform the desired operation. So the codebase is currently split between the two paradigms. A good ending state would be to move the math operation functions that currently take RATs/NUMs and instead have that logic be a part of the Rational and Number classes. Then we can finish converting the rest of the codebase to use Rational/Number and delete RAT/NUM entirely.