Correct handler range checks #235
Conversation
|
I think the extra_modes field could be deleted from member if everything other than validate mode was collapsed to a size of 0xf? |
|
Not sure if it really matters but moving index to after the other fields allows everything else to be aligned to 8 bytes. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #235 +/- ##
=======================================
Coverage 97.67% 97.67%
=======================================
Files 24 24
Lines 1074 1074
Branches 162 162
=======================================
Hits 1049 1049
Misses 12 12
Partials 13 13 |
|
I guess there's no need to add so many empty entries, think I should reduce some of them? |
|
I did my static assert's wrong.. it should be 16. |
|
Let me know when you are done making changes, and I will review. |
frmdstryr
force-pushed
the
handler-size-checks
branch
from
888d710 to
02a7da9
Compare
frmdstryr
force-pushed
the
handler-size-checks
branch
from
02a7da9 to
8b32c1b
Compare
|
Ok, I think it's good now. |
|
Since the Validate enum size is right at the limit of 5 bits do you think it should do a check on the handler size (in case someone tries to add something later)? |
atom/src/validatebehavior.cpp
Outdated
| @@ -1007,7 +1007,7 @@ handlers[] = { | |||
| PyObject* | |||
| Member::validate( CAtom* atom, PyObject* oldvalue, PyObject* newvalue ) | |||
| { | |||
| if( get_validate_mode() >= sizeof( handlers ) ) | |||
| if( get_validate_mode() >= sizeof( handlers ) / sizeof( handler ) ) | |||
There was a problem hiding this comment.
This looks different that the rest?
|
Are we making this change just to satisfy an IDE linter? |
|
I think all we really need to static assert is that the largest |
|
That way, things could still grow more handlers in the future if need-be. |
|
Agree, let me update it. |
frmdstryr
force-pushed
the
handler-size-checks
branch
from
2d20655 to
05967c0
Compare
|
Maybe we convert these C-arrays into C++ arrays: Then we don't have to do the division to compute the length. |
|
Good idea, do you want to take over or should I just update this? |
|
I think you are well equipped to do it. I'm just a gray-hair giving advice nowadays. |
frmdstryr
force-pushed
the
handler-size-checks
branch
from
c2a03c1 to
6797be7
Compare
atom/src/defaultvaluebehavior.cpp
Outdated
| if( get_default_value_mode() >= sizeof( handlers ) ) | ||
| return no_op_handler( this, atom ); // LCOV_EXCL_LINE | ||
| return handlers[ get_default_value_mode() ]( this, atom ); |
There was a problem hiding this comment.
Here you basically eliminated bound checks. It may be better to use std::array::at to benefit from the stdlib bound checks.
There was a problem hiding this comment.
The check is done when it parses the enum value, but I agree it's still possible to go OOB if the mode bits somehow get misparsed or that check is bypassed.
My goal of originally looking at this was to try and delete if's... I'd rather go back to re-adding no_op_handlers than add another if.
There was a problem hiding this comment.
std::array<handler, 8> doesn't give a compile error if you give less than 8 entries so I'd rather just go back to 8b32c1b with maybe an additional static_assert check on the sentinel?
There was a problem hiding this comment.
Can we statically assert that we do not have more enum variants than we have handlers? if we can do that we should be good.
atom/src/defaultvaluebehavior.cpp
Outdated
| @@ -187,8 +187,7 @@ typedef PyObject* | |||
| ( *handler )( Member* member, CAtom* atom ); | |||
|
|
|||
|
|
|||
| static handler | |||
| handlers[] = { | |||
| static const std::array<handler, DefaultValue::Mode::Last> handlers = { | |||
There was a problem hiding this comment.
I must be missing something but I cannot find in any doc that kind of syntax creation for an array (i.e. using a sentinel value). Could you share a reference ?
There was a problem hiding this comment.
I think it just implicitly casts the enum to size_t?
frmdstryr
force-pushed
the
handler-size-checks
branch
from
6797be7 to
0395936
Compare
frmdstryr
force-pushed
the
handler-size-checks
branch
from
0395936 to
3644c89
Compare
|
Yes, I'd like a stab at it. Please don't merge this yet. I will make another PR with a differing implementation of these same ideas. |
|
@sccolbert are you still planning to make an alternate version ? |
|
I think there is a cleaner way of doing this, but I dont have the time
right now. If it works and solves the current problem, I'm okay with it. We
can always improve it later.
…On Thu, Mar 6, 2025 at 3:38 PM Matthieu Dartiailh ***@***.***> wrote:
@sccolbert <https://github.com/sccolbert> are you still planning to make
an alternate version ?
—
Reply to this email directly, view it on GitHub
<#235 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABBQSLJUBCNTRYILDQHWK32TC54JAVCNFSM6AAAAABVSUJLZOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOMBUHE4TSNRUGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
[image: MatthieuDartiailh]*MatthieuDartiailh* left a comment
(nucleic/atom#235)
<#235 (comment)>
@sccolbert <https://github.com/sccolbert> are you still planning to make
an alternate version ?
—
Reply to this email directly, view it on GitHub
<#235 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABBQSLJUBCNTRYILDQHWK32TC54JAVCNFSM6AAAAABVSUJLZOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOMBUHE4TSNRUGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
My IDE was giving a warning because the if statements were always false.
After looking at it more it looks like it was accidentally doing a size instead of array size check.
Since all the modes except for validate behavior have less than 15 handlers I made it fill out the array with the default handler and cast the mode to a 0xf so no if statement is needed.