Closed thirtytwobits closed 5 months ago
@thirtytwobits
Thanks for taking time to contribute to FreeRTOS+TCP.
Since, we would like to avoid having compiler specific code in the core library files, selectively disabling the Wpedantic
check to the offending line is not possible. The CI checks gets around this problem by disabling the Wpedantic
check for the entire FreeRTOS_Sockets.c
.
Since the type of pvOptionValue
is something that's inherited from the BSD socket, its not trivial or backward compatible to fix it by a type change as well.
Since, we would like to avoid having compiler specific code in the core library files, selectively disabling the Wpedantic check to the offending line is not possible.
I understand the desire to avoid compiler-specific code however this is inert in all manifestations as it only guides GCC warnings and not actual compiler behaviour and is deactivated entirely for any other compiler. It may be ugly but it is, ultimately, benign.
Since the type of
pvOptionValue
is something that's inherited from the BSD socket, its not trivial or backward compatible to fix it by a type change as well.
Please consider that this library otherwise compiles with -pedantic
. This one line gets in the way of consumers that want to use this check. If you continue to disable -pedantic
in your CI more violations will creep in over time and it will be come more and more difficult to change your mind.
Would a version of this that abstracted "GCC" away be more palatable? Something like:
ipconfigISO_STRICTNESS_VIOLATION_START
pxSocket->pxUserWakeCallback = ( SocketWakeupCallback_t ) pvOptionValue;
ipconfigISO_STRICTNESS_VIOLATION_END
Users would then be able to redefine these as the necessary pragma push/pop on GCC:
#define ipconfigISO_STRICTNESS_VIOLATION_START _Pragma("GCC diagnostic push") \
_Pragma("GCC diagnostic ignored \"-Wpedantic\"")
#define ipconfigISO_STRICTNESS_VIOLATION_END _Pragma("GCC diagnostic pop")
@thirtytwobits
If you continue to disable -pedantic in your CI more violations will creep in over time and it will be come more and more difficult to change your mind.
Yes, disabling the warning for the entire file doesn't solve the problem.
What do you think about the following alternate option:
const void * pvCopySource = &pvOptionValue;
void * pvCopyDest = (void *) &( pxSocket->pxUserWakeCallback );
( void ) memcpy( pvCopyDest, pvCopySource, sizeof( SocketWakeupCallback_t ) );
instead of pxSocket->pxUserWakeCallback = ( SocketWakeupCallback_t ) pvOptionValue;
. This is similar to how Linux handles this.
Would a version of this that abstracted "GCC" away be more palatable? Something like: ipconfigISO_STRICTNESS_VIOLATION_START
Thats more abstract and cleaner when considering multiple compilers. But if possible we would still try to avoid compiler specific stuff in the code.
Hi @thirtytwobits, wondering if you prefer to update the PR with proposed changes, if you agree with them, or if you want us to make the changes.
Yeah, sorry. I owe you these proposed changes. I will get them to you this week.
Thanks @thirtytwobits
Opened a new PR #1140
Fixing GCC compiler warning
Description
This allows compilation of the socket source in projects using GCC with -pedantic= and -Werror. The change has no functional impact.
Test Steps
Compiles with GCC, -pedantic, and -Werror
Checklist:
Related Issue
(none)
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.