Skip to content

prevent adding to group a null item#25

Open
ferdymercury wants to merge 3 commits intomainfrom
nulls
Open

prevent adding to group a null item#25
ferdymercury wants to merge 3 commits intomainfrom
nulls

Conversation

@ferdymercury
Copy link
Copy Markdown
Collaborator

This fixes the nullptr access reported in #22 (not yet the memleaks)

@ferdymercury ferdymercury requested a review from martukas March 19, 2024 09:36
@ferdymercury ferdymercury mentioned this pull request Mar 19, 2024
Copy link
Copy Markdown
Owner

@martukas martukas left a comment

Choose a reason for hiding this comment

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

Sorry it took me a while to take a look at this. Hope you are still around? A few issues I noticed, but thanks, all of this looks like worthwhile improvements!

Comment thread source/SchemeEditor/LevelItem.h Outdated
Comment thread source/SchemeEditor/LevelItem.h Outdated
LevelItem *levrend = new LevelItem(level, LevelItem::DaughterLevelType,
parentpos_, visual_settings_, scene_);
connectItem(levrend);
delete daughter_levels_[level.energy()];
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

should we check to make sure it's non-null before deleting it?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the review!
if it's null, delete nullptr is a no-op, so it's not really necessary to check for it?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Any way to use RAII here to avoid having deletes alltogether? I don't recall how the Qt patterns might constrain this.

{
FeedingArrow* feed = new FeedingArrow(level, parentpos_, visual_settings_, scene_);
connectItem(feed);
delete feeding_arrows_[level.energy()];
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

also here?

Comment thread source/SchemeEditor/LevelItem.h Outdated
Comment thread source/SchemeEditor/LevelItem.h Outdated
Comment thread source/SchemeEditor/NuclideItem.h Outdated
LevelItem *levrend = new LevelItem(level, LevelItem::DaughterLevelType,
parentpos_, visual_settings_, scene_);
connectItem(levrend);
delete daughter_levels_[level.energy()];
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the review!
if it's null, delete nullptr is a no-op, so it's not really necessary to check for it?

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