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

Fix null tools #17048

Merged
merged 6 commits into from Jun 5, 2016

Conversation

Projects
None yet
3 participants
@mugling
Copy link
Contributor

commented Jun 4, 2016

Inspired by #17025 and fixes #16751. Tried submitting a PR for @Coolthulhu's repository but can't get it to work.

  • a650e6d is @Coolthulhu's migration for existing bugged saves
  • d372d16 stops items with integral magazines spawning with a null magazine when with_ammo
  • 4fe7f75 incidentally increases the robustness of item::ammo_set
@@ -1491,6 +1491,11 @@ void item::io( Archive& archive )
// only for backward compatibility (nowadays mode is stored in item_vars)
gun_set_mode(mode);
}

// Fixes #16751

This comment has been minimized.

Copy link
@Coolthulhu

Coolthulhu Jun 4, 2016

Contributor

Would be better if it described the bug, such as "Fixes bugged saves with tools containing nulls"

@Coolthulhu

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2016

It may be worth adding a is_null check in the emplace function (and debugmsg on null) to prevent it happening later on.

@mugling

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2016

@Coolthulhu I did consider this. Is it likely to prove a performance concern?

@Coolthulhu

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2016

No, is_null check is at worst a string comparison, emplace( item ) is an order of magnitude more expensive.

@mugling

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2016

@Coolthulhu see d001e67. We can't back out of adding a null item due to the return of a reference but we can add a debugmsg

@kevingranade kevingranade merged commit b8329da into CleverRaven:master Jun 5, 2016

1 check passed

default
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.