GDScript: Improve validation and documentation of @export_flags
#72493
Conversation
dalexeev
force-pushed
the
gds-export-flags
branch
from
e41283f
to
6c3d77e
Compare
| return false; | ||
| } | ||
| int64_t value = t[1].to_int(); | ||
| if (value < 1 || value >= (1LL << max_flags)) { |
There was a problem hiding this comment.
const int64_t max_flags = 32;
int64_t value = t[1].to_int();
value >= (1LL << max_flags)I'm not sure if this is correct.
There was a problem hiding this comment.
1LL << max_flags should be 4294967296 (2^32) which is correct, since the underlying type is a 32-bit integer. The >= comparison is also correct, since the value needs to be less than the maximum; otherwise, it would overflow and become 0.
There was a problem hiding this comment.
I'm not sure if int_64t is needed for the constant 32, but I haven't found another way to satisfy CI (tests on multiple platforms have failed).
There was a problem hiding this comment.
The value 1 << 32 doesn't fit in an 32-bit integer. So I'd say it needs to be stored in a int64_t. As for the max_flags constant, I don't think it needs to be int64_t, unless the compiler is complaining about mixing types in bitwise operations.
There was a problem hiding this comment.
It's all unsigned no? Usually for bitwise operations we use 1U << flags where flags is uint64_t.
There was a problem hiding this comment.
The 2^32 value would still not fit in a 32-bit unsigned integer. It will fit in both a signed and unsigned 64-bit integer. I don't know if it matters in C++ if the 64-bit integer is signed, but yes flags usually are unsigned.
dalexeev
force-pushed
the
gds-export-flags
branch
2 times, most recently
from
ebae500
to
483d5ae
Compare
modules/gdscript/gdscript_parser.cpp
Outdated
| if (p_annotation->name == SNAME("@export_flags") && p_annotation->resolved_arguments.size() > max_flags) { | ||
| push_error(vformat(R"(The argument count limit for "@export_flags" is exceeded (%d/%d).)", p_annotation->resolved_arguments.size(), max_flags), p_annotation); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
I think this code is no longer needed? Since the flag values are already checked below, and (in theory) there can be more than 32 arguments (different combinations of flags)?
There was a problem hiding this comment.
The value of each argument can't exceed 32^2 2^32 because the underlying type is a 32-bit integer. So this error is still useful because it helps avoid exceeding this limit when the arguments don't specify their value, although it could be improved.
There was a problem hiding this comment.
No, I mean something like:
A:1,B:2,C:4,D:8,E:16,F:32,G:64,<X>
Where <X> is all combinations of 3 flags. C(7,3) = 7! / (3! * 4!) = 35. So there are 7 + 35 = 42 arguments in total. This is greater than 32, but each of the arguments does not exceed the allowed value.
The only thing missing from the check below is the calculation of an auto value if :number is not specified.
Note: I haven't checked how the inspector handles this.
There was a problem hiding this comment.
My response was addressing your question I think this code is no longer needed?. I was thinking it was still needed for the case when values are not specified because it will increase the values automatically so the 33rd argument will exceed 2^32 (4294967296). But I totally missed that you also said Since the flag values are already checked below.
I think, when the value is not specified, this check would give a better error. Otherwise, it may be a bit confusing for users to get Invalid argument %d of annotation "@export_flags": The flag value must be at least 1 and at most 2 ** %d - 1. when they didn't specify any value.
There was a problem hiding this comment.
I think we can finalize this PR, and discuss further improvements separately.
There was a problem hiding this comment.
@export_flags(\
"F00:1", "F01:1", "F02:1", "F03:1",
"F04:1", "F05:1", "F06:1", "F07:1",
"F08:1", "F09:1", "F10:1", "F11:1",
"F12:1", "F13:1", "F14:1", "F15:1",
"F16:1", "F17:1", "F18:1", "F19:1",
"F20:1", "F21:1", "F22:1", "F23:1",
"F24:1", "F25:1", "F26:1", "F27:1",
"F28:1", "F29:1", "F30:1", "F31:1",
"F32:2", "F33:4", "F34:8", "F35:16"
) var test
dalexeev
force-pushed
the
gds-export-flags
branch
2 times, most recently
from
934a47b
to
911a9b3
Compare
dalexeev
force-pushed
the
gds-export-flags
branch
from
911a9b3
to
c8e3d8b
Compare
|
Thanks! |
@export_flagssupports specifying other values with a colon, just like@export_enum. See the comment.