-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Use Guild shared_ptr instead of raw pointers #4682
Conversation
Use const when possible
I definitely wouldn't remove the name |
I'm the opposite on this topic. I feel that seeing std::shared_ptr() in the code, everywhere it's used, gives a very clear indication to whomever is reading it, what it is, without having to think about it, or take the time to get the compiler to show its underlying structure. I also am opposite on the use of "auto" everywhere possible though too. I feel it should only really be used in certain situations, but for the most part, one should specify the type, so that whomever is reading it, know's what it is without having to think about it. That's just my opinion though. |
The problem is rather if you have I'm also not happy with the declaration of variables inside if statements, we never did that before and we shouldn't start doing it now, it kinda makes the code unreadable just for decreasing the line size by 1 each time, not worth the trade |
I prefer The use of Initializing variables within an if statement is already common in many parts of TFS. Not only does it restrict the variable's scope, but it also checks for nullptr right away. |
Nowadays, who uses the notes blog to work on a project? The visual studio or visual studio code linter immediately shows you the type, and saying that something is unlikely to happen as an excuse to do things one way is not a good justification. I could well say, well, since no one will change X thing, then I will add Y things... Also, whatever the case, in TFS the smart pointers each have their alias and this is how it should be done, respect the code style of the repository At least it is my personal opinion, if anyone else agrees or disagrees, please leave your opinion. ty |
I doubt anyone will test this pull request, and that's just how it goes. GitHub doesn't have a built-in linter, and this is where we review the code, LGTM. At this stage, using Regardless, I'll adhere to the established pattern. |
I'm 100% with @ramon-bernardo here. "if we ever have to change" OK but why would we ever? We probably have bigger fish to fry if we need to rename a class. About
Also read answer 3 if it piques your interest. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, one step forward to smart ptr usage.
I like the Guild_ptr better than std::shared_ptr<Guild>
, it's longer to write xD
Auto is fine, but I don't think you can use that in method arguments.
Since this is now a shared pointer, i guess we dont need the |
@maattch thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM too
After reviewing everything, it seems correct now. The last change for inline rank check |
🚀 |
Pull Request Prelude
Changes Proposed
auto
andconst auto
, at all, when possible.Guild::MEMBER_RANK_LEVEL_DEFAULT
.Discussion
Should we use
shared_ptr<T>
orT_ptr
at all?GuildWars
instead ofGuildWarVector
?