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

Add templated helper class to promote type-safety #965

Merged
merged 6 commits into from
Apr 12, 2019
Merged

Conversation

Headline
Copy link
Member

@Headline Headline commented Mar 9, 2019

Along with some type safety checks to ensure everything is kosher, this also technically fixes us relying on UB punning.

@Headline Headline added Bug general bugs; can be anything Needs Testing untested by author labels Mar 9, 2019

bool ret;
g_pAcceptInput->Execute(vstk, &ret);
MemWriter<void*, const char*, CBaseEntity*, CBaseEntity*, decltype(g_Variant_t), int> vstk(pDest, inputname, pActivator, pCaller, g_Variant_t, params[5]);
Copy link
Member Author

@Headline Headline Mar 9, 2019

Choose a reason for hiding this comment

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

in-case anyone's curious, this decltype(g_Variant_t) should be okay since it's a global char[] (not char*)

	char test[32];
	std::cout << sizeof(test) << std::endl; //32
	std::cout << sizeof(decltype(test)) << std::endl; //32

Copy link
Member

@KyleSanderson KyleSanderson left a comment

Choose a reason for hiding this comment

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

This is nice; but there's a multitude of style defects and I'd like to know why we're calling malloc when we can hopefully use stack storage instead.

public/sm_memwriter.h Outdated Show resolved Hide resolved
extensions/cstrike/util_cstrike.cpp Outdated Show resolved Hide resolved
extensions/cstrike/natives.cpp Show resolved Hide resolved
extensions/cstrike/natives.cpp Show resolved Hide resolved
extensions/cstrike/util_cstrike.cpp Show resolved Hide resolved
public/sm_memwriter.h Outdated Show resolved Hide resolved
public/sm_memwriter.h Outdated Show resolved Hide resolved
public/sm_memwriter.h Outdated Show resolved Hide resolved
public/sm_memwriter.h Outdated Show resolved Hide resolved
@Headline
Copy link
Member Author

Things should be a-okay now. Buffer is now statically allocated, renamed it per the suggestion to ArgBuffer, and whitespace stuff should be better. 🎉

Copy link
Member

@KyleSanderson KyleSanderson left a comment

Choose a reason for hiding this comment

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

The style is still wildly inconsistent 😢

@asherkin
Copy link
Member

This is a really nice quality-of-life improvement. Obviously it looks sane and should behave sanely, but has this been tested in-game with any of these calls?

@asherkin asherkin requested a review from KyleSanderson March 19, 2019 14:29
@Headline
Copy link
Member Author

Headline commented Mar 19, 2019

@asherkin no, but I’ve ensured through type-punning by hand that the buffer format is okay. I can do game testing as well to ensure

Copy link
Member

@KyleSanderson KyleSanderson left a comment

Choose a reason for hiding this comment

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

Please test in-game to ensure this doesn't break anything. Beyond that fundamentally it's an improvement; thanks for this.

@Headline Headline added Works For Me tested ok for author and removed Needs Testing untested by author labels Mar 21, 2019
@Headline
Copy link
Member Author

Everything seems to function just as before. Ran through CS_GetTranslatedWeaponAlias and made sure

Copy link
Member

@asherkin asherkin left a comment

Choose a reason for hiding this comment

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

@Headline is this waiting on anything else?

@Headline
Copy link
Member Author

@asherkin nope!

@Headline Headline merged commit e2eac38 into master Apr 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug general bugs; can be anything Works For Me tested ok for author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants