Comments (20)
They likely can be replaced with calls to make_unique
. For example:
PRAT tmp;
createrat(tmp);
can become:
auto tmp = make_unique<RAT>();
from calculator.
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?
from calculator.
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.
from calculator.
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];
from calculator.
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.
from calculator.
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:
- Use
unique_ptr<RAT>
when ownership of memory needs to be expressed. - Where possible, update ratpak functions to accept
const RAT&
orRAT&
. 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 aRAT&
. - Leave the remaining ratpak functions as accepting a
PRAT
and, given aunique_ptr<RAT> uniqueRat;
, call ratpack functions passinguniqueRat.get()
.
If there are problems where the ratpack functions are swapping pointers, we should update those functions to work with the smart pointer paradigms.
from calculator.
This is your friendly Microsoft Issue Bot. I created this issue automatically as requested by a team member.
from calculator.
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.
from calculator.
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?
from calculator.
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
from calculator.
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?)
from calculator.
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;
}
from calculator.
@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.
from calculator.
with the planned removal of create/delete rat/num
where should allocation of the structs be handled
from calculator.
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.
from calculator.
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.
from calculator.
if its worth anything delete[]
wont compile when used on MANTTYPE
, but ill look out for the rewrite and work from there
from calculator.
Yeah, delete[]
was a bad suggestion on my part. The memory isn't dynamically allocated.
from calculator.
@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?
from calculator.
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.
from calculator.
Related Issues (20)
- How to remove fucking spaces from this calculator or return to windows 7 verison. HOT 2
- space
- Wrong result in the win 11 calculator HOT 2
- Calculator Does Not Support Multiplying By Negative Numbers
- Using '%' in Standard Calculator Clears Input HOT 1
- Multiple Currency Conversion
- Calculator crashes if you zoom to far out on the Graphing Page
- Ja
- Ja
- [Localization]
- SWIPE DELETE FUNCTION FREEZES APP
- Wrong icon of calculator
- Disable scientific notation
- Programmer calculator treats MSB as sign bit, no option to remove sign bit
- Can you set a button for the top function in all (scientific) modes
- Is it possible to add an option that allows you to restore the history even after closing the application and opening it again? HOT 1
- "about equal to" feature in the volume calculator/converter
- Constant ratio of units on the axes
- [Localization] [zh-CN] Incorrect translations in Currency
- Disabling the Clipboard User Service causes Calculator to crash when copying values or pressing CTRL+C
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from calculator.