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

Fixed minor bugs #3256

Closed
wants to merge 1 commit into from
Closed

Fixed minor bugs #3256

wants to merge 1 commit into from

Conversation

GermanAizek
Copy link
Contributor

Detailed description

@XVilka hi,
I think this commit will be very useful in your project.
Undefined segments variable call clear() after moved value and replace new leak to garantee free unique_ptr

@karliss karliss self-requested a review October 31, 2023 20:46
@karliss
Copy link
Member

karliss commented Oct 31, 2023

Both of these changes are no good. Please do not spam static analyzer suggestions without spending minimum even minimum effort trying to understand what the code does are attempting to run and test the changes.

If you are actual beginner trying help, I will not take your contribution opportunity way and give you a week or two to fix the change. If something is unclear please ask questions.

Copy link
Member

@karliss karliss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get a feeling that you didn't even try to run the first change.

And you have already been pointed out in other merge requests why the second change pattern isn't correct. There is a room for improving the code, if you read how the structure is used, but I leave that as an exercise for you.

@karliss
Copy link
Member

karliss commented Nov 11, 2023

This is going nowhere I am closing the PR.

@karliss karliss closed this Nov 11, 2023
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.

2 participants