First version of agentic changes for removing conversion ˆ
Draft
nayakned wants to merge 1 commit into
Draft
Conversation
sign-conversion warnings. - Added -Wconversion and -Wsign-conversion to cmake build options - Explicit casts for getter functions for all data formats. Signed-off-by: Naresh Nayak <Naresh.Nayak@hs-furtwangen.de>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR enables -Wconversion and -Wsign-conversion in the library's CMake build options and adds explicit casts to getter functions across all AVTP data-format headers and source files (and a few related spots) to silence the resulting warnings. It also includes a small functional fix in Avtp_Vss_Pad (correcting struct-pointer arithmetic in memset) and tightens loop index types in the VSS array serialize/deserialize paths.
Changes:
- Add
-Wconversion -Wsign-conversionto both library andcustomlibcompile options. - Add explicit narrowing casts to all
Avtp_*_Get*getters (uint8/16/32/64, enum, bool) and several internal expressions inUtils.c. - Refactor
Avtp_Vss_Pad, VSS array (de)serialization, andexamples/common/common.csocket/timestamp code to silence implicit conversion warnings.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/CMakeLists.txt | Adds the two new warning flags to both library targets unconditionally. |
| src/avtp/Utils.c | Explicit (uint8_t)/(uint32_t) casts in Avtp_GetField/Avtp_SetField quadlet math. |
| src/avtp/CommonHeader.c, Crf.c, Rvf.c | Casts on getter return values. |
| src/avtp/aaf/Aaf.c, aaf/Pcm.c | Casts on getter return values, including AAF enum types. |
| src/avtp/cvf/Cvf.c, cvf/H264.c, cvf/Mjpeg.c, cvf/Jpeg2000.c | Casts on getter return values, including CVF format enums. |
| src/avtp/acf/custom/Vss.c | Casts in getters; padSize cast; memset pointer-arithmetic fix; new padded_length temp; loop indices changed from int to uint16_t; signed-array branches use new pointer casts. |
| include/avtp/acf/{Tscf,Ntscf,Sensor,SensorBrief,Most,Lin,Gpc,Gisf,Gbb,FlexRay,CanXl,CanXlBrief,CanV2,CanBriefV2,CanBrief,Can,AcfCommon,Abb}.h | Casts on inline getter return values; expression-wrapping casts in *GetPayloadLen/*GetLen. |
| examples/common/common.c | Casts using glibc-internal __time_t/__syscall_slong_t; htons((uint16_t)protocol/port) casts. |
Comments suppressed due to low confidence (3)
src/avtp/acf/custom/Vss.c:572
- Inconsistency in the new loop bound casts across the array branches: VSS_UINT16/UINT32/UINT64/FLOAT/DOUBLE wrap the divisor with
(uint16_t)(... / N), but the corresponding signed branches (VSS_INT16_ARRAY at line 543, VSS_INT32_ARRAY at line 557, VSS_INT64_ARRAY at line 571) omit the cast. Under -Wconversion this will leave warnings in exactly the cases this PR is meant to clean up. Apply the same(uint16_t)cast to the signed-array loop bounds for consistency.
for (uint16_t i = 0; i < val->data_int16_array->data_length/2; i++) {
*((int16_t*)(vss_data_ptr+2) + i) = (int16_t)Avtp_CpuToBe16(*(uint16_t*)(val->data_int16_array->data+i));
}
break;
case VSS_UINT32_ARRAY:
*(uint16_t*)vss_data_ptr = Avtp_CpuToBe16(val->data_uint32_array->data_length);
for (uint16_t i = 0; i < (uint16_t)(val->data_uint32_array->data_length/4); i++) {
*((uint32_t*)(vss_data_ptr+2) + i) = Avtp_CpuToBe32(*(val->data_uint32_array->data+i));
}
break;
case VSS_INT32_ARRAY:
*(uint16_t*)vss_data_ptr = Avtp_CpuToBe16(val->data_int32_array->data_length);
for (uint16_t i = 0; i < val->data_int32_array->data_length/4; i++) {
*((int32_t*)(vss_data_ptr+2) + i) = (int32_t)Avtp_CpuToBe32(*(uint32_t*)(val->data_int32_array->data+i));
}
break;
case VSS_UINT64_ARRAY:
*(uint16_t*)vss_data_ptr = Avtp_CpuToBe16(val->data_uint64_array->data_length);
for (uint16_t i = 0; i < (uint16_t)(val->data_uint64_array->data_length/8); i++) {
*((uint64_t*)(vss_data_ptr+2) + i) = Avtp_CpuToBe64(*(val->data_uint64_array->data+i));
}
break;
case VSS_INT64_ARRAY:
*(uint16_t*)vss_data_ptr = Avtp_CpuToBe16(val->data_int64_array->data_length);
for (uint16_t i = 0; i < val->data_int64_array->data_length/8; i++) {
*((int64_t*)(vss_data_ptr+2) + i) = (int64_t)Avtp_CpuToBe64(*(uint64_t*)(val->data_int64_array->data+i));
src/avtp/acf/custom/Vss.c:572
- Casting through a differently-signed pointer (
*(uint16_t*)(val->data_int16_array->data+i), and similar(uint32_t*)/(uint64_t*)casts in the INT32/INT64 branches) is unnecessary and risks pulling in strict-aliasing/UB issues; the underlying object already has an integer type compatible with the corresponding unsigned type, but the new pointer cast obscures intent. Casting the result ofAvtp_CpuToBeXX(which already takes/returns unsigned) and then assigning is sufficient; the operand can stay as*(val->data_int16_array->data+i)with an implicit conversion to uint16_t handled at the call site, or by an explicit value cast rather than a pointer cast. Please reconsider these pointer casts.
*((int16_t*)(vss_data_ptr+2) + i) = (int16_t)Avtp_CpuToBe16(*(uint16_t*)(val->data_int16_array->data+i));
}
break;
case VSS_UINT32_ARRAY:
*(uint16_t*)vss_data_ptr = Avtp_CpuToBe16(val->data_uint32_array->data_length);
for (uint16_t i = 0; i < (uint16_t)(val->data_uint32_array->data_length/4); i++) {
*((uint32_t*)(vss_data_ptr+2) + i) = Avtp_CpuToBe32(*(val->data_uint32_array->data+i));
}
break;
case VSS_INT32_ARRAY:
*(uint16_t*)vss_data_ptr = Avtp_CpuToBe16(val->data_int32_array->data_length);
for (uint16_t i = 0; i < val->data_int32_array->data_length/4; i++) {
*((int32_t*)(vss_data_ptr+2) + i) = (int32_t)Avtp_CpuToBe32(*(uint32_t*)(val->data_int32_array->data+i));
}
break;
case VSS_UINT64_ARRAY:
*(uint16_t*)vss_data_ptr = Avtp_CpuToBe16(val->data_uint64_array->data_length);
for (uint16_t i = 0; i < (uint16_t)(val->data_uint64_array->data_length/8); i++) {
*((uint64_t*)(vss_data_ptr+2) + i) = Avtp_CpuToBe64(*(val->data_uint64_array->data+i));
}
break;
case VSS_INT64_ARRAY:
*(uint16_t*)vss_data_ptr = Avtp_CpuToBe16(val->data_int64_array->data_length);
for (uint16_t i = 0; i < val->data_int64_array->data_length/8; i++) {
*((int64_t*)(vss_data_ptr+2) + i) = (int64_t)Avtp_CpuToBe64(*(uint64_t*)(val->data_int64_array->data+i));
src/avtp/acf/custom/Vss.c:93
- The PR description says "First version of agentic changes for removing conversion & sign-conversion warnings" and lists "Explicit casts for getter functions for all data formats", but the changes here go beyond getter casts: there are also functional-looking edits (e.g.
memset((uint8_t*)vss_pdu + ...)pointer-arithmetic fix inAvtp_Vss_Pad, the newpadded_lengthlocal, and signed-array pointer casts inAvtp_Vss_SetVssData). These are not pure cast-only changes and warrant a description update so reviewers know to look at them as logic changes rather than type-tightening. Thememsetchange in particular is a real bugfix (previouslyvss_pdu + vss_lengthwas struct-pointer arithmetic, advancing bysizeof(Avtp_Vss_t) * vss_lengthbytes), which deserves to be called out.
padSize = (uint8_t)((AVTP_QUADLET_SIZE - (vss_length % AVTP_QUADLET_SIZE)) % AVTP_QUADLET_SIZE);
if (vss_length % AVTP_QUADLET_SIZE) {
memset((uint8_t*)vss_pdu + vss_length, 0, padSize);
}
// Set the length and padding fields
padded_length = vss_length + padSize;
Avtp_Vss_SetField(vss_pdu, AVTP_VSS_FIELD_ACF_MSG_LENGTH,
(uint64_t)(padded_length / AVTP_QUADLET_SIZE));
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+99
to
+100
| tspec->tv_sec = (__time_t)(ptime / NSEC_PER_SEC); | ||
| tspec->tv_nsec = (__syscall_slong_t)(ptime % NSEC_PER_SEC); |
Collaborator
There was a problem hiding this comment.
The issue is that tspec nsec member type is not standardized: https://en.cppreference.com/c/chrono/timespec
But does this give a waring at all withput the long cast? And if so maybe we need to #ifdef it corretly for any platform we encounter
Comment on lines
+82
to
+93
| uint16_t padded_length; | ||
|
|
||
| // Check if padding is required | ||
| padSize = (AVTP_QUADLET_SIZE - (vss_length % AVTP_QUADLET_SIZE)) % AVTP_QUADLET_SIZE; | ||
| padSize = (uint8_t)((AVTP_QUADLET_SIZE - (vss_length % AVTP_QUADLET_SIZE)) % AVTP_QUADLET_SIZE); | ||
| if (vss_length % AVTP_QUADLET_SIZE) { | ||
| memset(vss_pdu + vss_length, 0, padSize); | ||
| memset((uint8_t*)vss_pdu + vss_length, 0, padSize); | ||
| } | ||
|
|
||
| // Set the length and padding fields | ||
| padded_length = vss_length + padSize; | ||
| Avtp_Vss_SetField(vss_pdu, AVTP_VSS_FIELD_ACF_MSG_LENGTH, | ||
| (uint64_t) (vss_length+padSize)/AVTP_QUADLET_SIZE); | ||
| (uint64_t)(padded_length / AVTP_QUADLET_SIZE)); |
Comment on lines
40
to
+52
| @@ -49,7 +49,7 @@ elseif(DEFINED ENV{ZEPHYR_BASE}) | |||
| add_library(${LIB_NAME}custom STATIC ${LIB_SOURCES}) | |||
| target_link_libraries(${LIB_NAME}custom PRIVATE zephyr_interface) | |||
| endif() | |||
| target_compile_options(${LIB_NAME}custom PRIVATE -Wall -Wextra) | |||
| target_compile_options(${LIB_NAME}custom PRIVATE -Wall -Wextra -Wconversion -Wsign-conversion) | |||
| uint32_t quadletHostOrder = Avtp_BeToCpu32(*quadletPtr); | ||
| uint32_t partialValue = (quadletHostOrder & quadletMask) >> quadletShift; | ||
| result |= (uint64_t)(partialValue) << (fieldDescriptor->bits - processedBits - quadletBits); | ||
| result |= (uint64_t)(partialValue) << ((uint32_t)(fieldDescriptor->bits - processedBits - quadletBits)); |
|
|
||
| sk_addr->sll_family = AF_PACKET; | ||
| sk_addr->sll_protocol = htons(protocol); | ||
| sk_addr->sll_protocol = htons((uint16_t)protocol); |
Collaborator
There was a problem hiding this comment.
Isn't the main question her rather, why the function signature itself ist not defining the datatype clearly? Why is it not uint16_t there?
This file contains hidden or 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
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.
sign-conversion warnings.