-
Notifications
You must be signed in to change notification settings - Fork 177
Full SM transitional syntax upconversion including methodmaps #483
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
Conversation
A few DataPack memory leaks patched Minor code tidying
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.
Good job 👍 Just need few edits
| #if SOURCEMOD_V_MAJOR >= 1 && SOURCEMOD_V_MINOR >= 8 | ||
| SetPackPosition(data, view_as<DataPackPos>(16)); | ||
| dataPack.Position = view_as<DataPackPos>(16); | ||
| #else |
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.
You can remove the sourcemod version check and use the latest, moving forward we are only going to permit 1.8+
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.
Roger that.
|
|
||
| #if defined DEBUG | ||
| decl String:auth[64]; | ||
| cgar auth[64]; |
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.
I too love cgar
| if (gagState && gagLength == 0) | ||
| { | ||
| return ThrowNativeError(SP_ERROR_NATIVE, "Permanent gag is not allowed!"); | ||
| ThrowNativeError(SP_ERROR_NATIVE, "Permanent gag is not allowed!"); |
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.
All ThrowNativeError should return to stop the execution of the rest
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.
Good catch.
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.
This is not required. Just compare implementations ThrowError() and ThrowNativeError().
| // MENU CODE // | ||
|
|
||
| public void OnAdminMenuReady(Handle topmenu) | ||
| public void OnAdminMenuReady(Handle hTemp) |
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.
This type could be TopMenu
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.
Never mind on this, apparently Sourcemod still uses Handle as the type for this. This could be a separate PR to Sourcemod to change this.
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.
I actually can't compile this with TopMenu. Looks like the compiler's forcing the Handle in the prototype:
function argument named 'topmenu' differs from prototype
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.
Yeah, that's a PR to Sourcemod, for another day
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.
This is not TopMenu handle. This is generic handle. Check API.
https://sm.alliedmods.net/new-api/topmenus/TopMenu/FromHandle
| public void OnAdminMenuReady(Handle topmenu) | ||
| public void OnAdminMenuReady(Handle hTemp) | ||
| { | ||
| TopMenu topmenu = view_as<TopMenu>(hTemp); |
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.
Then this casting wouldn't be needed
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.
Never mind on this, apparently Sourcemod still uses Handle as the type for this. This could be a separate PR to Sourcemod to change this.
|
Thanks for taking the time to review this long commit guys. 👍 |
Converted all pre-transitional code to comply with new SM syntax
Code has also been refactored to use method maps and their updated callbacks where applicable
A few DataPack memory leaks were also patched and minor code styles tidied up
Description
Method map usage include:
Motivation and Context
Improve readability with syntax standardization for ease of future debugging and development.
Types of changes
Compiler requirements have been upped to SM 1.7+. All compatibility legacy code for pre-transitional syntax compilers have been removed.
Checklist: