Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update CALC_E_* to no longer use macros #505

Closed
gsfreema opened this issue May 13, 2019 · 4 comments · Fixed by #710
Closed

Update CALC_E_* to no longer use macros #505

gsfreema opened this issue May 13, 2019 · 4 comments · Fixed by #710

Comments

@gsfreema
Copy link
Contributor

@gsfreema gsfreema commented May 13, 2019

Problem Statement
One of the goals for this project appears to be converting legacy C style code to modern C++. Avoiding macros unless completely necessary has been a C++ best practice for quite some time.

Proposal
With more modern features such as constexpr values, you should be able to replace all CALC_E_* macros with constexpr values. Preferably, you could replace them with an enum class, but that is riskier and would require much more code refactoring.

Goals
Modernize the code base and make it easier to maintain.

@msftbot msftbot bot added this to Pitch in Feature Tracking May 13, 2019
@MicrosoftIssueBot

This comment has been minimized.

Copy link

@MicrosoftIssueBot MicrosoftIssueBot commented May 13, 2019

This is your friendly Microsoft Issue Bot. I've seen this issue come in and have gone to tell a human about it.

@Grommers00

This comment has been minimized.

Copy link
Contributor

@Grommers00 Grommers00 commented Oct 10, 2019

Hi I'd like to attempt this. Is that okay?

@HowardWolosky

This comment has been minimized.

Copy link
Member

@HowardWolosky HowardWolosky commented Oct 10, 2019

Thanks for your interest, @Grommers00. Go for it!

@Grommers00

This comment has been minimized.

Copy link
Contributor

@Grommers00 Grommers00 commented Oct 11, 2019

Okay awesome. I think I have a recommended solution.

Source of the Issue located here

I recommend we change the variables to read like this
image
As errors can go for the full length of the program.

I do not recommend this approach
image
As it can be wasteful to memory, and may cause additional unforeseen problems. (e.g. if Errors themselves crashed out.)

As for testing for this particular fix, I was able to run the program with no errors (A ton of unrelated warnings though!). However, I am unsure on how to directly test this particular issue out.

https://github.com/Grommers00/calculator/tree/E_CODE_Update/src/CalcManager

Is the current location of the branch I would like to push.

Is this acceptable?

@Grommers00 Grommers00 mentioned this issue Oct 11, 2019
@EriWong EriWong closed this in #710 Oct 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.