Feature/fixing clang gnu compiler warnings #45

phelter · 2023-05-01T20:03:09Z

Fixing Clang and GNU compiler warnings along with some clang-tidy issues.

Description

Upkeep/maintenance - fixed a bunch of the compiler warnings that were previously ignored. See lists in CMakeLists.txt for ones fixed.

Test Steps

None - re-run test suite.

Checklist:

[x ] I have tested my changes. No regression in existing tests.
[x ] I have modified and/or added unit-tests to cover the code changes in this Pull Request.

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.

compilation.

…ments related to (FreeRTOS#35)

a modified xsdps driver that requires it.

…ed code for removing failure in 2032 due to uint32_t used for time.

include/ff_error.h

phelter · 2023-05-01T20:06:25Z

CMakeLists.txt

@@ -186,22 +186,16 @@ target_compile_definitions( freertos_plus_fat

 target_compile_options( freertos_plus_fat
  PRIVATE
+    $<$<COMPILE_LANG_AND_ID:C,GNU>:-Wno-array-bounds>


This is compiling with clang 14.0

Nit: I feel array-bounds violations are fairly useful warnings, but I'm unaware whether there were intended "false positive" types being reported. Regardless plenty of other warnings were fixed, so approved first round of reviews.

I built POSIX test on Ubuntu 22.04 with GCC11 with Warray-bounds and no array bound warnings came up.

@dachalco I also use GCC11.3.0 and had to compile with that added to ensure no errors. I treat all warnings as errors. So this list is merely stating what is detected by Clang and GNU when compiling with those.

If they are no longer there then feel free to delete them, but the process I typically use is:

identify everything first the linters/compilers show up

Remove the warnings I intend to fix,

Fix the warnings until those compile without error.

If you have a build that confirms this is no longer there then feel free to remove once the workflow adds those.

phelter · 2023-05-01T20:47:21Z

/bot run uncrustify

dachalco · 2023-05-03T20:41:48Z

CMakeLists.txt

@@ -186,22 +186,16 @@ target_compile_definitions( freertos_plus_fat

 target_compile_options( freertos_plus_fat
  PRIVATE
+    $<$<COMPILE_LANG_AND_ID:C,GNU>:-Wno-array-bounds>


Nit: I feel array-bounds violations are fairly useful warnings, but I'm unaware whether there were intended "false positive" types being reported. Regardless plenty of other warnings were fixed, so approved first round of reviews.

dachalco · 2023-05-03T20:55:44Z

CMakeLists.txt

@@ -186,22 +186,16 @@ target_compile_definitions( freertos_plus_fat

 target_compile_options( freertos_plus_fat
  PRIVATE
+    $<$<COMPILE_LANG_AND_ID:C,GNU>:-Wno-array-bounds>


I built POSIX test on Ubuntu 22.04 with GCC11 with Warray-bounds and no array bound warnings came up.

dachalco · 2023-05-03T21:22:42Z

@phelter
I tried to push a commit to fix the failing formatting checks but did not have permissions for this PR.
To fix the failing formatting checks, the failing files need to be uncrustified (v0.69).

htibosch

Hello @phelter, you did a very good job!

One more comment about a line that is not visible here below:

-    if( uxIndex == 0 )
+    if( uxIndex == 0U )
     {
-         uxIndex = 1;
+         uxIndex = 1U;
     }

that is because you made xIndex unsigned:

-    BaseType_t xIndex;
+    UBaseType_t uxIndex;

I approve the changes, thank you.

htibosch · 2023-05-03T08:33:41Z

ff_file.c


        FF_PendSemaphore( pxIOManager->pvSemaphore );
        {
-            for( xIndex = 0; xIndex < ffconfigPATH_CACHE_DEPTH; xIndex++ )
+            for( UBaseType_t xIndex = 0; xIndex < ffconfigPATH_CACHE_DEPTH; xIndex++ )


I also like the declaration within a for loop, but some users might still be using an older C dialect than C99?
And also, when xIndex becomes unsigned, it should be called uxIndex.

htibosch · 2023-05-03T22:07:13Z

ff_format.c

@@ -252,13 +252,13 @@ static FF_Error_t prvFormatGetClusterSize( struct xFormatSet * pxSet,

    for( ; ; )
    {
-        int32_t groupSize;
+        uint32_t groupSize;


Change it to ulGroupSize?

feel free to change once this has been updated. I truly don't see the point in adding superfluous prefixes to values to identify the type when the IDE's these days can tell you that with pop-ups or other features. My 2 cents but if that is the format or flow then I suggest you add a tool to check and fix those for you since it is a simple thing to do automatically.
https://clang.llvm.org/docs/ClangFormatStyleOptions.html

I understand you use uncrustify for this process, but it seems as though the bots for doing that aren't working.

ff_ioman.c

ff_stdio.c

htibosch · 2023-05-03T22:21:57Z

ff_stdio.c

@@ -2167,10 +2167,9 @@ int prvFFErrorToErrno( FF_Error_t xError )

 #if ( ffconfigTIME_SUPPORT == 1 )

-    static uint32_t prvFileTime( FF_SystemTime_t * pxTime )
+    time_t prvFileTime( FF_SystemTime_t * pxTime )


Here above prvFileTime() was declared private. I don't think that the function is useful outside this module.

@htibosch

A note about C and C++ - you only need to declare the function as static or extern for the declaration. So yes above it is declared as private in the declaration (where it should be). This is the definition, and in the definition you do not need to duplicate the static or extern.

Also for C - you can have a declaration of:

static void func(void); ///< Declaration ... void func() ///< Definition { }

You can remove the void in the definition.

include/ff_stdio.h

include/ff_error.h

portable/ATSAM4E/ff_sddisk.c

htibosch · 2023-05-03T22:46:49Z

portable/STM32F4xx/ff_sddisk.c

               SD_state,
               prvSDCodePrintable( ( uint32_t ) SD_state ),
               xSDHandle.CardType == HIGH_CAPACITY_SD_CARD ? "SDHC" : "SD",
-               xSDCardInfo.CardCapacity / ( 1024 * 1024 ) );
+               (unsigned)(xSDCardInfo.CardCapacity / ( 1024 * 1024 ) ));


As CardCapacity i unsigned, use 1024U * 1024U here?

Yes that could be done in lieu of this change.

htibosch · 2023-05-03T22:50:49Z

ff_file.c

            {
-                BaseType_t len2 = STRLEN( pxIOManager->xPartition.pxPathCache[ xIndex ].pcPath );
+                UBaseType_t len2 = STRLEN( pxIOManager->xPartition.pxPathCache[ xIndex ].pcPath );


And replace len2 with uxLength2?

Goal was not to fix the names of variables to match their types. Goal was to use the correct types.

htibosch · 2023-05-03T23:07:49Z

/bot run uncrustify

htibosch · 2023-05-03T23:10:58Z

Why not create a new PR which will "uncrustify" the source code?

phelter

The ci build bot for performing uncrustify is broken when merging from a fork. Please provide the proper method for executing uncrustify, or fix the bot.

I have no intention on installing uncrustify as I consider it a subset of the tools that I already use (clang-format) and have no use for it other than to fix code in freertos.

phelter · 2023-05-04T04:54:09Z

CMakeLists.txt

@@ -186,22 +186,16 @@ target_compile_definitions( freertos_plus_fat

 target_compile_options( freertos_plus_fat
  PRIVATE
+    $<$<COMPILE_LANG_AND_ID:C,GNU>:-Wno-array-bounds>


@dachalco I also use GCC11.3.0 and had to compile with that added to ensure no errors. I treat all warnings as errors. So this list is merely stating what is detected by Clang and GNU when compiling with those.

If they are no longer there then feel free to delete them, but the process I typically use is:

identify everything first the linters/compilers show up

Remove the warnings I intend to fix,

Fix the warnings until those compile without error.

If you have a build that confirms this is no longer there then feel free to remove once the workflow adds those.

phelter · 2023-05-04T04:57:55Z

ff_stdio.c

@@ -2167,10 +2167,9 @@ int prvFFErrorToErrno( FF_Error_t xError )

 #if ( ffconfigTIME_SUPPORT == 1 )

-    static uint32_t prvFileTime( FF_SystemTime_t * pxTime )
+    time_t prvFileTime( FF_SystemTime_t * pxTime )


@htibosch

A note about C and C++ - you only need to declare the function as static or extern for the declaration. So yes above it is declared as private in the declaration (where it should be). This is the definition, and in the definition you do not need to duplicate the static or extern.

Also for C - you can have a declaration of:

static void func(void); ///< Declaration ... void func() ///< Definition { }

You can remove the void in the definition.

ff_stdio.c

phelter · 2023-05-04T05:06:10Z

ff_time.c

@@ -105,8 +104,8 @@
    #define WEEK_DAY_SATURDAY       6

 /* Make a bitmask with a '1' for each 31-day month. */
-    #define _MM( month )    ( 1u << ( month - 1 ) )
-    #define MASK_LONG_MONTHS     ( _MM( 1 ) | _MM( 3 ) | _MM( 5 ) | _MM( 7 ) | _MM( 8 ) | _MM( 10 ) | _MM( 12 ) )
+    #define MM_( month )    ( 1u << ( month - 1 ) )


@htibosch - macros and precompiler definitions that start with _ are considered reserved and should not be used.
https://clang.llvm.org/docs/DiagnosticsReference.html#wreserved-identifier

For MISRA checks - if they were run in the ci.yml then they pass. If not then I suggest if this is a requirement then you should add it to the ci.yml to check that.

phelter · 2023-05-04T05:08:10Z

ff_time.c

@@ -141,7 +140,7 @@

    static portINLINE unsigned long ulDaysPerYear( int iYear )


Yes I would suggest that, but out of scope of this particular PR to reduce those specific compiler warnings.

Also using size_t and uintptr_t for sizes and addresses is also best.

phelter · 2023-05-04T05:16:39Z

ff_format.c

@@ -252,13 +252,13 @@ static FF_Error_t prvFormatGetClusterSize( struct xFormatSet * pxSet,

    for( ; ; )
    {
-        int32_t groupSize;
+        uint32_t groupSize;


feel free to change once this has been updated. I truly don't see the point in adding superfluous prefixes to values to identify the type when the IDE's these days can tell you that with pop-ups or other features. My 2 cents but if that is the format or flow then I suggest you add a tool to check and fix those for you since it is a simple thing to do automatically.
https://clang.llvm.org/docs/ClangFormatStyleOptions.html

I understand you use uncrustify for this process, but it seems as though the bots for doing that aren't working.

ff_ioman.c

phelter · 2023-05-04T05:22:29Z

portable/STM32F4xx/ff_sddisk.c

               SD_state,
               prvSDCodePrintable( ( uint32_t ) SD_state ),
               xSDHandle.CardType == HIGH_CAPACITY_SD_CARD ? "SDHC" : "SD",
-               xSDCardInfo.CardCapacity / ( 1024 * 1024 ) );
+               (unsigned)(xSDCardInfo.CardCapacity / ( 1024 * 1024 ) ));


Yes that could be done in lieu of this change.

phelter · 2023-05-04T05:22:57Z

ff_file.c

            {
-                BaseType_t len2 = STRLEN( pxIOManager->xPartition.pxPathCache[ xIndex ].pcPath );
+                UBaseType_t len2 = STRLEN( pxIOManager->xPartition.pxPathCache[ xIndex ].pcPath );


Goal was not to fix the names of variables to match their types. Goal was to use the correct types.

moninom1 · 2023-05-10T08:57:47Z

/bot run uncrustify

phelter · 2023-05-10T16:09:14Z

@moninom1

/bot run uncrustify

The bot is not working I've tried, and so have others. I don't know if it has something to do with this being a merge from a fork?

moninom1 · 2023-05-10T17:50:40Z

@phelter Yeah i checked. Adding changes to fix the issue in a separate PR. Will update once merged.

moninom1 · 2023-05-11T07:08:35Z

/bot run uncrustify

htibosch · 2023-05-11T15:14:11Z

+1
Just to make sure, I approve @phelter's PR, let's merge it.
Thank you

aggarg · 2023-05-15T12:25:05Z

test/build-combination/Common/main.c

@@ -35,13 +35,11 @@
 #include "task.h"

 /* System application includes. */
+#include "FreeRTOSConfig.h"


This should not be needed explicitly - including "FreeRTOS.h" is enough.

+1
The first thing FreeRTOS.h does is :

/* Application specific configuration options. */ #include "FreeRTOSConfig.h"

Description ----------- #45 introduced a compilation error about implicit `FF_CreateError` function when using FAT12 support. I assume it was supposed to be `FF_createERR` macro instead so I changed it. Checklist: ----------   - [x] I have tested my changes. No regression in existing tests. - [ ] I have modified and/or added unit-tests to cover the code changes in this Pull Request. By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

phelter added 12 commits October 31, 2022 20:12

Support for CMake builds. Added Linux stub port for now to allow cross

06bb5a6

compilation.

Fixing misuse of doxygen documentation

321db9e

Converting prints from %lu to %u where possible as per request in com…

b583a73

…ments related to (FreeRTOS#35)

Fixing clang compiler warnings.

15c8f65

Fixing build test to work regardless of configuration.

bdc2f5a

Adding in for Zynq port dependency on uncached_memory target.

c095b29

Excluding build_test from all build - must request it explicitly.

c6e8021

Ensuring XPAR_XSDPS_0_IS_CACHE_COHERENT is 1 for this build since using

5163366

a modified xsdps driver that requires it.

Updating compiler warnings.

105f900

Fixing documentation, signed/unsigned conversion. Bugfix for time bas…

a37b5f0

…ed code for removing failure in 2032 due to uint32_t used for time.

Merge branch 'master' into feature/fixing-clang-gnu-compiler-warnings

8ff184c

Fixing error creation using FF_createERR everywhere missed.

3f8fb14

phelter requested a review from a team as a code owner May 1, 2023 20:03

phelter commented May 1, 2023

View reviewed changes

include/ff_error.h Outdated Show resolved Hide resolved

phelter commented May 1, 2023

View reviewed changes

Fixing build_test in ci.yml

4aa9dbb

dachalco previously approved these changes May 3, 2023

View reviewed changes

htibosch previously approved these changes May 3, 2023

View reviewed changes

phelter commented May 4, 2023

View reviewed changes

Merge branch 'main' into feature/fixing-clang-gnu-compiler-warnings

325a308

Uncrustify: triggered by comment.

d500500

github-actions bot dismissed htibosch’s stale review via d500500 May 11, 2023 07:09

github-actions bot dismissed dachalco’s stale review via d500500 May 11, 2023 07:09

moninom1 approved these changes May 11, 2023

View reviewed changes

amazonKamath requested review from htibosch and moninom1 May 11, 2023 07:17

moninom1 approved these changes May 11, 2023

View reviewed changes

moninom1 self-requested a review May 11, 2023 16:37

moninom1 approved these changes May 11, 2023

View reviewed changes

moninom1 requested review from n9wxu and htibosch and removed request for htibosch May 11, 2023 17:04

aggarg reviewed May 15, 2023

View reviewed changes

aggarg approved these changes May 15, 2023

View reviewed changes

kar-rahul-aws approved these changes May 16, 2023

View reviewed changes

n9wxu merged commit 08d0cff into FreeRTOS:main May 16, 2023

bjsowa mentioned this pull request Jan 29, 2024

Fix usage of FF_createERR macro in prvGetFAT12Entry #62

Merged

2 tasks

		@@ -141,7 +140,7 @@

		static portINLINE unsigned long ulDaysPerYear( int iYear )

Feature/fixing clang gnu compiler warnings #45

Feature/fixing clang gnu compiler warnings #45

Conversation

phelter commented May 1, 2023

Description

Test Steps

Checklist:

Related Issue

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phelter commented May 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dachalco commented May 3, 2023

htibosch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htibosch commented May 3, 2023

htibosch commented May 3, 2023

phelter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moninom1 commented May 10, 2023

phelter commented May 10, 2023

moninom1 commented May 10, 2023

moninom1 commented May 11, 2023

htibosch commented May 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment