Conversation
There was a problem hiding this comment.
Pull Request Overview
Adds support for a new TRICK_ENUMERATED attribute type, handling it similarly to existing integral types, while explicitly excluding it from logical time de/encoding.
- Add TRICK_ENUMERATED to initialize and is_supported_attribute_type switch cases
- Handle TRICK_ENUMERATED in byteswap_buffer_copy the same as TRICK_INTEGER (32-bit)
- No changes to logical time encode/decode paths
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } | ||
| break; | ||
| } | ||
| case TRICK_ENUMERATED: |
There was a problem hiding this comment.
[nitpick] This treats TRICK_ENUMERATED as a 32-bit int by casting to int. If TRICK_ENUMERATED is not guaranteed to be 32-bit across platforms, this can produce incorrect byte-swapping. Consider either (a) documenting and guarding the assumption with a compile-time check (e.g., a static_assert that TRICK_ENUMERATED maps to a 4-byte encoding), or (b) selecting the swap width based on the attribute's encoded size (2/4/8 bytes) to make this portable.
| case TRICK_ENUMERATED: | |
| case TRICK_ENUMERATED: { | |
| // Swap based on the actual size of TRICK_ENUMERATED. | |
| size_t elem_size = num_bytes / length; | |
| if (elem_size == sizeof(short)) { | |
| short const *e_src = static_cast<short const *>(src); | |
| short *e_dest = static_cast<short *>(dest); | |
| for (int k = 0; k < length; ++k) { | |
| e_dest[k] = Utilities::byteswap_short(e_src[k]); | |
| } | |
| } else if (elem_size == sizeof(int)) { | |
| int const *e_src = static_cast<int const *>(src); | |
| int *e_dest = static_cast<int *>(dest); | |
| for (int k = 0; k < length; ++k) { | |
| e_dest[k] = Utilities::byteswap_int(e_src[k]); | |
| } | |
| } else if (elem_size == sizeof(long long)) { | |
| long long const *e_src = static_cast<long long const *>(src); | |
| long long *e_dest = static_cast<long long *>(dest); | |
| for (int k = 0; k < length; ++k) { | |
| e_dest[k] = Utilities::byteswap_long_long(e_src[k]); | |
| } | |
| } else { | |
| // Unknown size, just copy. | |
| memcpy(dest, src, num_bytes); | |
| } | |
| break; | |
| } |
There was a problem hiding this comment.
I think this is a good comment. Looking at the trick project, here is their assumption about TRICK_ENUMERATED. Not a full list, and while there appears to be some cases where the enum could be something other than int, it seems more often than not TRICK_ENUMERATED is assumed to be exclusively int.
- int: https://github.com/nasa/trick/blob/6ff28150d90f7b73bbf90f87de8b97d7be691e89/trick_source/data_products/Log/TrickBinary.cpp#L238
- int or short: https://github.com/nasa/trick/blob/6ff28150d90f7b73bbf90f87de8b97d7be691e89/trick_source/sim_services/MemoryManager/MemoryManager_clear_memory.cpp#L43
- int: https://github.com/nasa/trick/blob/6ff28150d90f7b73bbf90f87de8b97d7be691e89/trick_source/web/CivetServer/src/VariableServerVariable.cpp#L122
- int or short or char: https://github.com/nasa/trick/blob/6ff28150d90f7b73bbf90f87de8b97d7be691e89/trick_source/sim_services/MemoryManager/MemoryManager_ref_assignment.cpp#L101
- int or short: https://github.com/nasa/trick/blob/6ff28150d90f7b73bbf90f87de8b97d7be691e89/trick_source/sim_services/CheckPointAgent/ClassicCheckPointerAgent.cpp#L497
- int: https://github.com/nasa/trick/blob/6ff28150d90f7b73bbf90f87de8b97d7be691e89/trick_source/sim_services/DataRecord/DRHDF5.cpp#L116
- int: https://github.com/nasa/trick/blob/6ff28150d90f7b73bbf90f87de8b97d7be691e89/trick_source/sim_services/VariableServer/VariableReference.cpp#L485
- int and uint: https://github.com/nasa/trick/blob/6ff28150d90f7b73bbf90f87de8b97d7be691e89/trick_source/sim_services/MemoryManager/ref_to_value.c#L119
| case TRICK_LONG: | ||
| case TRICK_UNSIGNED_LONG: | ||
| case TRICK_LONG_LONG: | ||
| case TRICK_UNSIGNED_LONG_LONG: { | ||
| case TRICK_UNSIGNED_LONG_LONG: | ||
| case TRICK_ENUMERATED: { |
There was a problem hiding this comment.
[nitpick] TRICK_ENUMERATED is grouped with long/long long here, but in byteswap_buffer_copy it is grouped with TRICK_INTEGER. If the behavior is intended to mirror TRICK_INTEGER, consider grouping consistently (or add a brief comment) to avoid confusion about the expected width/semantics for TRICK_ENUMERATED in different code paths.
Adding in the TRICK_ENUMERATED type as if it was a TRICK_INTEGER type. (However, not including it to the logical time de/encoding)
Could you check to make sure this isn't a naïve change? I am not able to test this yet, but for now just mean to prompt the conversation and will update here when I have it tested.
#181