-
Notifications
You must be signed in to change notification settings - Fork 93
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 stronger tag checks to streamer.inc #435
Open
Y-Less
wants to merge
1
commit into
samp-incognito:master
Choose a base branch
from
Y-Less:patch-1
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
## Change List This makes several changes to tags: 1. Everything that accepts a boolean value (only `0` or `1`) now have `bool:` tags, possibly defaulting to `true` or `false`. 2. Everything that returns a boolean (`Is`/`Has` functions) now have `bool:` tags. 3. Several other functions that return success/fail also have `bool:` tags, but fewer are done. 4. The `STONG_TAGS`/`WEAK_TAGS`/`NO_TAGS` defines have been adopted from samp-stdlib. 5. `STREAMER_MAX_TYPES` and the type definitions have been converted to a proper enum. 6. `STREAMER_MAX_AREA_TYPES` and the type definitions have been converted to a proper enum. 7. `STREAMER_MAX_OBJECT_TYPES` and the type definitions have been converted to a proper enum. 8. `OBJECT_MATERIAL_SIZE:`, `MAPICON:`, `OBJECT_MATERIAL_TEXT_ALIGN:`, and `CP_TYPE:` optional tags support added. ## New Warnings These changes may or may not affect existing code, depending on how the code was written. Code already using the defines from the include should not be affected: ```pawn // This will now give a tag mismatch warning on the first parameter: Streamer_GetFloatData(0, objectid, E_STREAMER_R_X, dest); // This will continue to work correctly as before in all cases: Streamer_GetFloatData(STREAMER_TYPE_OBJECT, objectid, E_STREAMER_R_X, dest); ``` When using the tag overhauled samp/open.mp includes some functions now require the existing defines: ```pawn // This will now give a tag mismatch warning on the last parameter only if using the enhanced SA:MP includes: SetDynamicObjectMaterialText(objectid, materialindex, "text", .textalignment = 0); // This will continue to work correctly as before in all cases: SetDynamicObjectMaterialText(objectid, materialindex, "text", .textalignment = OBJECT_MATERIAL_TEXT_ALIGN_LEFT); ``` Additionally some functions have new defines within the enhanced includes: ```pawn // This will now give a tag mismatch warning on the first parameter only if using the enhanced SA:MP includes: CreateDynamicRaceCP(3, 100.0, 134.0, 77.0, 145.0, 167.0, 83.0, 5.0); // The new code in that case would look like: CreateDynamicRaceCP(CP_TYPE_AIR_NORMAL, 100.0, 134.0, 77.0, 145.0, 167.0, 83.0, 5.0); ``` Possibly the most intrusive change is the boolean parameters: ```pawn // This will now give a tag mismatch warning on the final parameter: Streamer_ToggleAllItems(playerid, STREAMER_TYPE_AREA, 1); // The previous code was idiomatic, but should now become: Streamer_ToggleAllItems(playerid, STREAMER_TYPE_AREA, true); ``` While everything above gave warnings, there is one breaking change - i.e introduces errors. This is unfortunate because logically tag mismatches between forward declarations and function definitions should also be a warning not an error, I don't know why the compiler implements them as the latter. Because the streamer type now has a tag, the callbacks that take a streamer type must also have that tag: ```pawn forward Streamer_OnItemStreamIn(STREAMER_TYPE:type, STREAMER_ALL_TAGS:id, forplayerid); forward Streamer_OnItemStreamOut(STREAMER_TYPE:type, STREAMER_ALL_TAGS:id, forplayerid); ``` Which means the publics become: ```pawn public Streamer_OnItemStreamIn(STREAMER_TYPE:type, STREAMER_ALL_TAGS:id, forplayerid) { } public Streamer_OnItemStreamOut(STREAMER_TYPE:type, STREAMER_ALL_TAGS:id, forplayerid) { } ``` Continuing to use the old version below will now give "function heading differs from prototype" errors: ```pawn public Streamer_OnItemStreamIn(type, STREAMER_ALL_TAGS:id, forplayerid) { } public Streamer_OnItemStreamOut(type, STREAMER_ALL_TAGS:id, forplayerid) { } ``` Fortunately there is a way to write code that works with both the old and new declarations, and that's to define the tag as "no tag" when this new include version isn't used: ```pawn #if !defined STREAMER_TYPE #define STREAMER_TYPE: _: #endif public Streamer_OnItemStreamIn(STREAMER_TYPE:type, STREAMER_ALL_TAGS:id, forplayerid) { } public Streamer_OnItemStreamOut(STREAMER_TYPE:type, STREAMER_ALL_TAGS:id, forplayerid) { } ``` When the `STREAMER_TYPE:` tag exists this code will use it. When it doesn't it won't. ## Explanation Warnings are *good* things: ```pawn // This code is probably incorrect, but there's no obvious way to tell: Streamer_GetDistanceToItem(0.0, 1.0, 2.0, 5, areaid, distance); // But after fixing tag mismatch warning, the mistake becomes clear: Streamer_GetDistanceToItem(0.0, 1.0, 2.0, STREAMER_TYPE_3D_TEXT_LABEL, areaid, distance); ``` ```pawn // This code is incorrect - global objects are type 0: AttachDynamicAreaToObject(areaid, globalObjectID, .type = 1); // This code is incorrect - player objects are type 1: AttachDynamicAreaToObject(areaid, playerObjectID, .type = 2); // This code is incorrect - dynamic objects are type 2 (3 doesn't exist): AttachDynamicAreaToObject(areaid, dynamicObjectID, .type = 3); ``` Using the correct names will fix the bug instantly because they are right by definition ```pawn // This code is correct by definition: AttachDynamicAreaToObject(areaid, globalObjectID, .type = STREAMER_OBJECT_TYPE_GLOBAL); // This code is correct by definition: AttachDynamicAreaToObject(areaid, playerObjectID, .type = STREAMER_OBJECT_TYPE_PLAYER); // This code is correct by definition: AttachDynamicAreaToObject(areaid, dynamicObjectID, .type = STREAMER_OBJECT_TYPE_DYNAMIC); ``` Or replacing the numbers with their exact names will show the problem instantly: ```pawn // The bug here becomes obvious from the symbol names: AttachDynamicAreaToObject(areaid, globalObjectID, .type = STREAMER_OBJECT_TYPE_PLAYER); // The bug here becomes obvious from the symbol names: AttachDynamicAreaToObject(areaid, playerObjectID, .type = STREAMER_OBJECT_TYPE_DYNAMIC); // This line can't even be completed because there's no define for object type 3: AttachDynamicAreaToObject(areaid, dynamicObjectID, .type = ???); ``` Using strict typing strongly encourages people to use the named defines all the time to enable the compiler to help with hard to spot mistakes. ## Configuration For whatever reason some people hate warnings and would rather turn them off than actually use them to improve their code (I assume they think that no warnings is more important than no bugs). This despite the fact that warnings do not prevent compilation at all and can be ignored if you just squint a bit. Anyway, for the sake of those people, these new tags can be disabled. There are actually three levels of warning enhancements: ### `WEAK_TAGS` This is the default and gives warnings when untagged values are passed to tagged values (e.g. passing `1` instead of `STREAMER_TYPE_PICKUP`). Basically everything documented above. So named because this is pawn *weak tag* semantics, achieved by prefixing all the new tags with a lower-case letter (`t_`). ```pawn // This is the default, to encourage adoption. //#define WEAK_TAGS #include <a_samp> #include <streamer> ``` ### `STRONG_TAGS` In addition to the weak tag warnings this adds warnings when tagged values are passed to untagged values. The only place this really matters in the streamer include is `GetDynamicAreaType`, which is currently the only function with a new tag type for its return value (I may have missed some others, feel free to point them out). So named because this is pawn *strong tag* semantics, achieved by prefixing all the new tags with an upper-case letter (`T_`). Old code, no warning: ```pawn #include <a_samp> #include <streamer> new type = GetDynamicAreaType(areaid); ``` New weak code, no warning: ```pawn #define WEAK_TAGS #include <a_samp> #include <streamer> new type = GetDynamicAreaType(areaid); ``` New strong code, tag mismatch warning: ```pawn #define STRONG_TAGS #include <a_samp> #include <streamer> new type = GetDynamicAreaType(areaid); ``` New strong code, fixed (will also work in all other cases): ```pawn #define STRONG_TAGS #include <a_samp> #include <streamer> new STREAMER_AREA_TYPE:type = GetDynamicAreaType(areaid); ``` ### `NO_TAGS` Disables all the new tags (except `bool:` currently): ```pawn #define NO_TAGS #include <a_samp> #include <streamer> // No warning. new STREAMER_AREA_TYPE:type1 = GetDynamicAreaType(areaid); // No warning. new type2 = GetDynamicAreaType(areaid); // No warning CreateDynamicMapIcon(1043.0, -436.0, 5.0, 7, 0xFF0000FF, .style = 2); // No warning CreateDynamicMapIcon(1043.0, -436.0, 5.0, 7, 0xFF0000FF, .style = MAPICON_GLOBAL_CHECKPOINT); ``` ## See Also https://github.com/pawn-lang/samp-stdlib/tree/consistency-overhaul#tags Note that this branch has long been accepted for merge, but still isn't. It will be soon.
Does this fix include file 2.9.4 and 2.9.5 causing the amx to die on Edit: nope, it doesn't for me ;\ |
I've never seen that error, I don't know what causes it sorry. |
AmyrAhmady
force-pushed
the
master
branch
3 times, most recently
from
October 15, 2024 17:21
2796929
to
dd2074b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Change List
This makes several changes to tags:
0
or1
) now havebool:
tags, possibly defaulting totrue
orfalse
.Is
/Has
functions) now havebool:
tags.bool:
tags, but fewer are done.STRONG_TAGS
/WEAK_TAGS
/NO_TAGS
defines have been adopted from samp-stdlib.STREAMER_MAX_TYPES
and the type definitions have been converted to a proper enum.STREAMER_MAX_AREA_TYPES
and the type definitions have been converted to a proper enum.STREAMER_MAX_OBJECT_TYPES
and the type definitions have been converted to a proper enum.OBJECT_MATERIAL_SIZE:
,MAPICON:
,OBJECT_MATERIAL_TEXT_ALIGN:
, andCP_TYPE:
optional tags support added.New Warnings
These changes may or may not affect existing code, depending on how the code was written.
Code already using the defines from the include should not be affected:
When using the tag overhauled samp/open.mp includes some functions now require the existing defines:
Additionally some functions have new defines within the enhanced includes:
Possibly the most intrusive change is the boolean parameters:
While everything above gave warnings, there is one breaking change - i.e introduces errors. This is unfortunate because logically tag mismatches between forward declarations and function definitions should also be a warning not an error, I don't know why the compiler implements them as the latter (Edit: operator overloads will give codegen problems). Because the streamer type now has a tag, the callbacks that take a streamer type must also have that tag:
Which means the publics become:
Continuing to use the old version below will now give "function heading differs from prototype" errors:
Fortunately there is a way to write code that works with both the old and new declarations, and that's to define the tag as "no tag" when this new include version isn't used:
When the
STREAMER_TYPE:
tag exists this code will use it. When it doesn't it won't.Explanation
Warnings are good things:
Using the correct names will fix the bug instantly because they are right by definition
Or replacing the numbers with their exact names will show the problem instantly:
Using strict typing strongly encourages people to use the named defines all the time to enable the compiler to help with hard to spot mistakes.
Configuration
For whatever reason some people hate warnings and would rather turn them off than actually use them to improve their code (I assume they think that no warnings is more important than no bugs). This despite the fact that warnings do not prevent compilation at all and can be ignored if you just squint a bit. Anyway, for the sake of those people, these new tags can be disabled.
There are actually three levels of warning enhancements:
WEAK_TAGS
This is the default and gives warnings when untagged values are passed to tagged values (e.g. passing
1
instead ofSTREAMER_TYPE_PICKUP
). Basically everything documented above. So named because this is pawn weak tag semantics, achieved by prefixing all the new tags with a lower-case letter (t_
).STRONG_TAGS
In addition to the weak tag warnings this adds warnings when tagged values are passed to untagged values. The only place this really matters in the streamer include is
GetDynamicAreaType
, which is currently the only function with a new tag type for its return value (I may have missed some others, feel free to point them out). So named because this is pawn strong tag semantics, achieved by prefixing all the new tags with an upper-case letter (T_
).Old code, no warning:
New weak code, no warning:
New strong code, tag mismatch warning:
New strong code, fixed (will also work in all other cases):
NO_TAGS
Disables all the new tags (except
bool:
currently):See Also
https://github.com/pawn-lang/samp-stdlib/tree/consistency-overhaul#tags
Note that this branch has long been accepted for merge, but still isn't. It will be soon.