Misra compliance #79

sanderh255 · 2022-01-03T08:25:35Z

Please find my MISRA modifications in this pull request.
I've tried to stick to the MISRA issues and not change other things ;)

I'm open for discussion and/or feedback!

…reprocessor directives or comments

This reverts commit e0aff68.

…ther a preceding &, or with a parenthesized parameter list, which may be empty.

… be used in place of the basic types

eyalroz · 2022-01-03T09:17:13Z

src/printf/printf.h

@@ -86,7 +102,7 @@ void putchar_(char character);
 * @param format A string that specifies the format of the output
 * @return The number of characters that are written into the array, not counting the terminating null character
 */
-int printf_(const char* format, ...) ATTR_PRINTF(1, 2);
+printf_int_t printf_(const char* format, ...) ATTR_PRINTF(1, 2);


This is not possible. The printf() functions must be compatible with the standard library. If the MISRA standard is incompatible with using the standard library, then we have a problem.

This is not necessarily incompatible with the std lib. If you're not defining PRINTF_CUSTOM_INT_xx then printf_int_t is a plain int. For MISRA compatibility you'd define PRINTF_CUSTOM_INT_32 to make printf_int_t an int32_t.

Ah, ok. I guess we can do something like that. But - I still have to see the literal int in the signature. So I would rather have something like:

#ifndef PRINTF_CUSTOM_INT_xx int printf_(const char* format, ...) ATTR_PRINTF(1, 2); #else printf_int_t printf_(const char* format, ...) ATTR_PRINTF(1, 2); #endif

or maybe even:

#ifndef PRINTF_CUSTOM_INT_xx int def1 int def2 int def3 #else printf_int_t def1 printf_int_t def2 printf_int_t def3 #endif

I've update the header file. Is this better?

You'll need a new PR with whatever changes remain after my next push.

Also... what does MISRA have to say about the standard library version of these functions? I mean, they return a plain int, after all.

Actually, I've just read the MISRA rule regarding typedefs. It says:

6.3 (adv) typedefs that indicate size and signedness should be used in place of the basic numerical types.

So, first of all, it's a "should", not a "shall" or "will". But even more importantly - the changes you propose don't bring you much closer to satisfying this rule, since the typedef you proposed indicates neither size nor signedness. I believe this advisory is about preferring int32_t, uint8_t and such over int, unsigned, char and such.

Ah ok. I'll see about that new pull request.

Basically MISRA indicates (among others) to avoid the standard library methods regarding time/date, character handling, stream operators and allocation/deallocation. It more or less recognizes one cannot do without, but discourages dependence.

More to the point, it says "For example, the essential type rules apply to the types of the arguments passed to functions specified in the standard library and to their results."

Actually, I've just read the MISRA rule regarding typedefs. It says:

6.3 (adv) typedefs that indicate size and signedness should be used in place of the basic numerical types.

So, first of all, it's a "should", not a "shall" or "will". But even more importantly - the changes you propose don't bring you much closer to satisfying this rule, since the typedef you proposed indicates neither size nor signedness. I believe this advisory is about preferring int32_t, uint8_t and such over int, unsigned, char and such.

I completely agree that is what the rule is about.
That's what I'm doing by defining printf_int_t as int16_t , int32_t or int64_t. This way the return value uses a type with defined signedness and size.

Basically MISRA indicates (among others) to avoid the standard library methods regarding time/date, character handling, stream operators and allocation/deallocation. It more or less recognizes one cannot do without, but discourages dependence.

Well, this library is about providing these same problematic functions. But - we'll see how it looks in the new PR. Closing this one for now.

eyalroz · 2022-01-03T09:21:51Z

src/printf/printf.c

@@ -261,11 +260,11 @@ static inline void out_wrapped_function(char character, void* wrapped_function,

 // internal secure strlen
 // @return The length of the string (excluding the terminating 0) limited by 'maxsize'
-static inline unsigned int strnlen_s_(const char* str, size_t maxsize)
+static inline printf_uint_t strnlen_s_(const char* str, size_t maxsize)


Maybe I could just make it size_t?

Good suggestion, done in an additional commit.

eyalroz · 2022-01-03T09:22:47Z

src/printf/printf.c

  }
  return i;
 }


 // output the specified string in reverse, taking care of any zero-padding
-static size_t out_rev_(out_fct_type out, char* buffer, size_t idx, size_t maxlen, const char* buf, size_t len, unsigned int width, unsigned int flags)
+static size_t out_rev_(out_fct_type out, char* buffer, size_t idx, size_t maxlen, const char* buf, size_t len, printf_uint_t width, printf_uint_t flags)


I suppose I could have a distinct type for the flag holder integer.

eyalroz · 2022-01-03T09:40:07Z

src/printf/printf.c

@@ -1055,8 +1056,9 @@ static int _vsnprintf(out_fct_type out, char* buffer, const size_t maxlen, const
            }
          }
          // string output
-          while ((*p != 0) && (!(flags & FLAGS_PRECISION) || precision--)) {
+          while ((*p != 0) && (!(flags & FLAGS_PRECISION) || precision)) {


Is this exactly equivalent?

What if there are no precision flags? In that case, with the new code, precision gets decremented while before it wasn't.

That is true, but precision is only used to exit the while loop, so this does not impact any functionality.

eyalroz · 2022-01-03T09:43:24Z

MISRA C:2004, 6.3 - typedefs that indicate size and signedness should be used in place of the basic types

Mostly ok, but not when it comes to standard library compliance of the user-visible functions. See individual comments. Also, I might want to have more than one unsigned-int replacement, depending on the context.

MISRA C:2004, 19.10

Ok.

MISRA C++:2008, 18-0-1 - The C library shall not be used.

Can you explain the impact of this change? I think I'm ok with it, but I'm not sure I understand exactly what would happen.

MISRA C:2004, 16.9 - A function identifier shall only be used with either a preceding &, or with a parenthesized parameter list, which may be empty.

Ok.

Unmodified pointers should be const (MISRA C:2004, 16.7 )

I believe you are mis-applying the rule. There is a common signature to these functions, and buffer is generally modified.
You seem to have two patches with this title, one saying "revert".

MISRA C:2012, 20.1 - #include directives should only be preceded by preprocessor directives or comments

Ok.

Removed redundant cast

Ok.

Merge collapsible if statements (not strictly MISRA)

Don't have a strong opinion here; I probably separated for readability and consistency with other pieces of code earlier or below. Can you explain why you believe this is useful/important?

Rule 13.3 increment/decrement with other side effects

Generally ok, but see specific comment

eyalroz · 2022-01-03T09:44:05Z

Oh, one last thing - your PR should be against the develop branch :-P

sanderh255 · 2022-01-03T11:37:48Z

Thanks for you comments, please find my responses below.

MISRA C:2004, 6.3 - typedefs that indicate size and signedness should be used in place of the basic types

Mostly ok, but not when it comes to standard library compliance of the user-visible functions. See individual comments. Also, I might want to have more than one unsigned-int replacement, depending on the context.

Sure, can you indicate which contexts you would like?

MISRA C:2004, 19.10

Ok.

MISRA C++:2008, 18-0-1 - The C library shall not be used.

Can you explain the impact of this change? I think I'm ok with it, but I'm not sure I understand exactly what would happen.

For this change, only when using C++, we're including the c++ version of the stdlib headers (so for example cstdarg instead of stdarg.h). The impact is compiler dependent, but most C++ compilers simply wrap the C versions so this should be negligible.

MISRA C:2004, 16.9 - A function identifier shall only be used with either a preceding &, or with a parenthesized parameter list, which may be empty.

Ok.

Unmodified pointers should be const (MISRA C:2004, 16.7 )

I believe you are mis-applying the rule. There is a common signature to these functions, and buffer is generally modified.

You seem to have two patches with this title, one saying "revert".

Correct, I came to same conclusion and reverted the commit.

MISRA C:2012, 20.1 - #include directives should only be preceded by preprocessor directives or comments

Ok.

Removed redundant cast

Ok.

Merge collapsible if statements (not strictly MISRA)

Don't have a strong opinion here; I probably separated for readability and consistency with other pieces of code earlier or below. Can you explain why you believe this is useful/important?

Mostly readability. Subsequent if statements imply more complexity than is actually present.

Rule 13.3 increment/decrement with other side effects

Generally ok, but see specific comment

I've changed the target to develop (still getting used to the github UI :) ).

eyalroz · 2022-01-05T18:42:22Z

So, I'm going to go ahead and cherry-pick some of this PR. The rest will need a bit of updating for other changes.

JakeSays · 2022-01-07T23:47:05Z

I'm little confused here. Couldn't MISRA compliance be met by using stdint.h types instead of a hack like printf_int_t?

eyalroz · 2022-01-08T07:21:02Z

@JakeSays : Not unless I decided to force people to use non-native integer types (e.g. by choosing int32_t always,. or int64_t always).

JakeSays · 2022-01-08T10:52:26Z

@eyalroz I'm not sure I follow. isn't stdint.h a standard? Personally I'd rather be forced to use int32_t instead of printf_int_t.

eyalroz · 2022-01-08T11:39:56Z

Personally I'd rather be forced to use int32_t instead of printf_int_t.

You won't see it anyway; the functions' interface remains the same as that of the standard. So why do you mind?
Some people have 16-bit native-int platforms. Although... it's not like libprintf is ultra-fast to the degree that this makes a difference.

JakeSays · 2022-01-08T12:25:35Z

What? Why wouldn't I see it? It's in the header and the source.
"MISRA C:2004, 6.3 - typedefs that indicate size and signedness should be used in place of the basic types" - wouldn't this require int32_t over int? Unless you want MISRA compliance to be optional.

My concern is this: One of the major attractions of libprintf is it's simplicity. The code is simple and easy to understand, and the header is a dream to read. It seems like many of these PR's are taking it in a different direction and it'll end up in #ifdef/#define hell.

eyalroz · 2022-01-08T12:59:31Z

What? Why wouldn't I see it? It's in the header and the source.

In this PR it is, but I don't think I would let it into the header. The fctprintf variants will return an int just like their standard library friends.

I still haven't seen an alternative PR. What I have done and committed to the code is drastically reduce the use of non-typedef'ed types in printf.c, using printf_size_t for widths, precision values etc.

* Fixes eyalroz#79. * Fixed a few more (GCC) warnings. * Separated the warning-triggering cases into test cases of their own. * Not using `signed size_t` - that's apparently not in the standard and not widely supported. * Added a CMake option for whether or not to test the warning-triggering cases.

sander.hagendoorn added 10 commits December 24, 2021 21:40

Rule 13.3 increment/decrement with other side effects

740b878

Merge collapsible if statements (not strictly MISRA)

7398ba0

Unmodified pointers should be const (MISRA C:2004, 16.7 )

e0aff68

Removed redundant cast

62bcd9d

MISRA C:2012, 20.1 - #include directives should only be preceded by p…

f5c7327

…reprocessor directives or comments

Revert "Unmodified pointers should be const (MISRA C:2004, 16.7 )"

c43886f

This reverts commit e0aff68.

MISRA C:2004, 16.9 - A function identifier shall only be used with ei…

950c40e

…ther a preceding &, or with a parenthesized parameter list, which may be empty.

MISRA C++:2008, 18-0-1 - The C library shall not be used.

7d74780

MISRA C:2004, 19.10

bee6c0d

MISRA C:2004, 6.3 - typedefs that indicate size and signedness should…

a6cb311

… be used in place of the basic types

eyalroz reviewed Jan 3, 2022

View reviewed changes

Change to size_t for internal strnlen return value

1dfd730

sanderh255 changed the base branch from master to develop January 3, 2022 11:13

eyalroz force-pushed the develop branch from 0267db6 to e11965e Compare January 5, 2022 18:30

sander.hagendoorn and others added 2 commits January 5, 2022 21:11

Split function declarations into standard and custom

cdeac63

Merge branch 'develop' into MISRA-compliance

a993682

sanderh255 closed this Jan 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Misra compliance #79

Misra compliance #79

sanderh255 commented Jan 3, 2022

eyalroz Jan 3, 2022

sanderh255 Jan 3, 2022

eyalroz Jan 5, 2022

sanderh255 Jan 5, 2022

eyalroz Jan 5, 2022 •

edited

Loading

eyalroz Jan 5, 2022

sanderh255 Jan 5, 2022

sanderh255 Jan 5, 2022 •

edited

Loading

eyalroz Jan 5, 2022

eyalroz Jan 3, 2022

sanderh255 Jan 3, 2022

eyalroz Jan 3, 2022

eyalroz Jan 3, 2022

sanderh255 Jan 3, 2022

eyalroz commented Jan 3, 2022

eyalroz commented Jan 3, 2022

sanderh255 commented Jan 3, 2022

eyalroz commented Jan 5, 2022

JakeSays commented Jan 7, 2022

eyalroz commented Jan 8, 2022

JakeSays commented Jan 8, 2022

eyalroz commented Jan 8, 2022

JakeSays commented Jan 8, 2022

eyalroz commented Jan 8, 2022

Misra compliance #79

Misra compliance #79

Conversation

sanderh255 commented Jan 3, 2022

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

eyalroz Jan 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanderh255 Jan 5, 2022 • edited Loading

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

eyalroz commented Jan 3, 2022

eyalroz commented Jan 3, 2022

sanderh255 commented Jan 3, 2022

eyalroz commented Jan 5, 2022

JakeSays commented Jan 7, 2022

eyalroz commented Jan 8, 2022

JakeSays commented Jan 8, 2022

eyalroz commented Jan 8, 2022

JakeSays commented Jan 8, 2022

eyalroz commented Jan 8, 2022

eyalroz Jan 5, 2022 •

edited

Loading

sanderh255 Jan 5, 2022 •

edited

Loading