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.

Sign up for GitHub

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

Jump to bottom

Make weapon private #50286

Merged
merged 23 commits into from
Sep 12, 2021
Merged

Make weapon private #50286

merged 23 commits into from
Sep 12, 2021

Conversation

Fris0uman
Copy link
Contributor

@Fris0uman Fris0uman commented Jul 28, 2021 •
edited
Loading

Summary

Infrastructure "Make weapon private"

Purpose of change

Make weapon member of Character private

Describe the solution

Describe alternatives you've considered

Testing

Compile without error

Can wield stuff, drop stuff, throw stuff, stab zombies without issues

Additional context

@actual-nh actual-nh added [C++] Changes (can be) made in C++. Previously named `Code` Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style labels Jul 28, 2021
@Fris0uman Fris0uman requested a review from KorGgenT July 29, 2021 05:54
@Qrox
Copy link
Contributor

Qrox commented Jul 29, 2021

get_wielded_item might be a better name because you can also wield non-weapon items.

@Fris0uman
Copy link
Contributor Author

get_wielded_item might be a better name because you can also wield non-weapon items.

Yes that'd make sens. I think once I have this in a proper shape I'll do a mass rename to get_wielded_item

GoLoT
GoLoT reviewed Jul 29, 2021
View reviewed changes
src/character.cpp Outdated Show resolved Hide resolved
GoLoT
GoLoT reviewed Jul 29, 2021
View reviewed changes
src/character.cpp Outdated Show resolved Hide resolved
@Fris0uman Fris0uman force-pushed the private_weapon branch 2 times, most recently from e9d0e3c to 4a8fb44 Compare August 14, 2021 08:13
@Fris0uman Fris0uman marked this pull request as ready for review August 15, 2021 08:31
kevingranade
kevingranade reviewed Aug 27, 2021
View reviewed changes
src/npcmove.cpp Outdated Show resolved Hide resolved
BrettDong
BrettDong reviewed Sep 2, 2021
View reviewed changes
src/avatar_action.cpp Outdated Show resolved Hide resolved
BrettDong
BrettDong reviewed Sep 2, 2021
View reviewed changes
src/bionics.cpp Outdated Show resolved Hide resolved
BrettDong
BrettDong reviewed Sep 2, 2021
View reviewed changes
src/crafting.cpp Outdated Show resolved Hide resolved
@anothersimulacrum
Copy link
Member

There are merge conflicts.

@Fris0uman Fris0uman marked this pull request as draft September 11, 2021 18:34
@Fris0uman Fris0uman marked this pull request as ready for review September 11, 2021 18:54
@kevingranade kevingranade merged commit da590d8 into CleverRaven:master Sep 12, 2021
@Fris0uman Fris0uman deleted the private_weapon branch September 12, 2021 15:51
@actual-nh
Copy link
Contributor

actual-nh commented Sep 12, 2021 •
edited
Loading

@Fris0uman: Android builds seem to be having a problem with this. First noticed in https://github.com/CleverRaven/Cataclysm-DDA/runs/3581520161, but also present right after this PR was merged.

@eltank
Copy link
Contributor

eltank commented Sep 13, 2021

This PR overlooked the code in sdltiles.cpp inside a giant section guarded by #if defined(__ANDROID__). I'm surprised that there are no CI builds that define that symbol. It's a significant gap in build verification.

@eltank
Copy link
Contributor

eltank commented Sep 13, 2021

I also don't understand why get_wielded_item() returns a const reference in the const version and a pointer in the non-const version (instead of a mutable reference). This is confusing API.

@eltank eltank mentioned this pull request Sep 13, 2021
Merged
@Fris0uman Fris0uman mentioned this pull request Sep 14, 2021
Merged
@eltank eltank mentioned this pull request Sep 15, 2021
Closed
Venera3 pushed a commit to Venera3/Cataclysm-DDA that referenced this pull request Sep 21, 2021
@Fris0uman @Venera3
Make weapon private (CleverRaven#50286)
b10c4bf 
* Make `weapon` private

* Update tests

* Rebase

* Appley suggestions

* Fix weapon dorping

* fix weapon saving

* Rename to get_wielded_item

* rename to set_wielded_item

* fixes

* Post rebase fixes

* fixes

* fix weapon saving

* fixes

* more rebase and fixes

* Apply suggestions

* post rebase fixes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BrettDong BrettDong BrettDong left review comments

@GoLoT GoLoT GoLoT left review comments

@kevingranade kevingranade kevingranade left review comments

@KorGgenT KorGgenT Awaiting requested review from KorGgenT

Assignees
No one assigned
Labels
[C++] Changes (can be) made in C++. Previously named `Code` Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@Fris0uman @Qrox @anothersimulacrum @actual-nh @eltank @kevingranade @GoLoT @BrettDong