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

stability improvements #185

Merged
merged 4 commits into from
Sep 15, 2019
Merged

stability improvements #185

merged 4 commits into from
Sep 15, 2019

Conversation

cre4ture
Copy link
Contributor

I'm planning to bring in more changes from my side. But before doing so I need to get used to the pull request idea (sorry I'm new here).

I chose to bring in two smaller crash fixes first which don't do any functionality change, but rather inprove stability. Even though I guess that an average user will anyway not have those situations, I had both cases locally on my PC during development.

Also there is this additional small refactoring of replacing the new and delete with std::unique_ptr apporach.

cre4ture added 4 commits July 22, 2019 21:12
makes code simpler and eliminates risk for memory leak
c++14 needed only for make_unique. unique_ptr already available with c++11
@madmaxoft
Copy link
Contributor

You merged master changes instead of rebasing on top of them, which created the merge commit in your branch instead of re-applying the changes in your branch on top of the current master branch. Not a big deal, but if you want the merge to be cleanly applied (without merge commit), you'd need to use rebase.

Also, it's usually a good idea to ask around before bumping the language version :)

@EtlamGit
Copy link
Collaborator

I think using build-in language features to implement smart pointers is fine. The rest of the code base uses Qt shared pointers, which is doing more or less the same. Maybe it is a future task to refactor the complete code base to use the same type of smart pointers.

Is there a real need to use C++14?
As far as I know, most of the smart pointer stuff is also present in C++11.
We support many different platforms, so I would try to be conservative with compiler requirements.

Regarding Pull Requests:
It is a good practice to do one feature per Pull Request.
This one is about stability of JSON parsing, so it is fine.
Like madmaxoft wrote, rebase is also an important git feature. But sometimes it get messed up. In these cases it helps to start with a clean fork of mrkite/master, create a new branch and copy your changes into this new directory.

@madmaxoft
Copy link
Contributor

I believe the only change in this PR requiring C++14 is std::make_unique .

@mrkite
Copy link
Owner

mrkite commented Sep 15, 2019

I think using build-in language features to implement smart pointers is fine. The rest of the code base uses Qt shared pointers, which is doing more or less the same. Maybe it is a future task to refactor the complete code base to use the same type of smart pointers.

The reason to use Qt shared pointers over the standard shared pointers is because Qt includes all that extra code to serialize QtSharedPtrs. You can then stuff them into QVariants and use them in signals and slots, etc.

However, there isn't a qtuniqueptr.. so if you want to use single-owner ptrs, you have to use c++14.

@mrkite mrkite merged commit fea8541 into mrkite:master Sep 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants