Add natives to work with 64 bit Protobuf values#943
Add natives to work with 64 bit Protobuf values#943KyleSanderson merged 6 commits intoalliedmodders:masterfrom komashchenko:master
Conversation
asherkin
left a comment
asherkin
left a comment
There was a problem hiding this comment.
This looks pretty good - one inline about alignment requirements that would be good to address (but isn't strictly required.)
core/smn_protobuf.cpp
Outdated
| GET_FIELD_NAME_OR_ERR(); | ||
|
|
||
| int64 *ret; | ||
| pCtx->LocalToPhysAddr(params[3], reinterpret_cast<cell_t **>(&ret)); |
There was a problem hiding this comment.
This isn't strictly legal as cell_t and int64 have differing alignment requirements (4 and 8 bytes respectively) and this could end up unaligned. (Which is just a perf penalty on x86, but can be fatal on other architectures.)
It would be better to use a temporary int64 variable and combine manually.
This comment applies generally throughout the PR.
|
@asherkin As I understand, do not need to add legacy API ? |
core/smn_protobuf.cpp
Outdated
| REGISTER_NATIVES(protobufnatives) | ||
| { | ||
| {"PbReadInt", smn_PbReadInt}, | ||
| {"PbReadInt64", smn_PbReadInt64}, |
There was a problem hiding this comment.
You missed those from the non-methodmap api.
|
Add this function is very necessary |
KyleSanderson
left a comment
KyleSanderson
left a comment
There was a problem hiding this comment.
I don't run this game anymore unfortunately but this looks good if the code works. I don't like the macro hell but that's existing so: not your problem. If you can use c++ casting instead of c that would be appreciated but beyond that this looks good bud and I'll merge in a day regardless.
Thanks for being patient.
| cell_t *value; | ||
| pCtx->LocalToPhysAddr(params[3], &value); | ||
| int64 temp; | ||
| ((cell_t *)&temp)[0] = value[0]; |
5 participants
No description provided.