JSC: general code cleanup

[skliper]

Originally implemented as part of trac #45 and isolated for CCB review purposes.

General code clean up modifications:

• Make all if/then/else as compound statements
• add "void" to functions that do not take parameters
• add final "else" to all "else if" constructs
• make sure all cases in switch stametents have break
• add explicit casting where the compiler may emit warnings
• Add "static" and "extern" keywords where needed

[skliper]

Imported from trac issue 118. Created by jphickey on 2015-11-06T17:27:04, last modified: 2015-12-09T11:13:56

[skliper]

Trac comment by jphickey on 2015-11-09 12:19:50:

Commit [changeset:1f9e588] is ready for CCB review. Branch trac-118-jsc-general-cleanup

[skliper]

Trac comment by jphickey on 2015-11-09 12:20:33:

I have reviewed this and recommend ACCEPT. Changes look benign, assuming this is the coding standard(s) that CCB wants to adopt.

[skliper]

Trac comment by sstrege on 2015-11-16 18:08:35:

Approve/accept

[skliper]

Trac comment by sduran on 2015-11-17 11:05:21:

recommend accept

[skliper]

Trac comment by sstrege on 2015-11-18 13:40:29:

11/10/15 CCB: Need to ensure use of "VOIDFUNCPTR" is safe before accepting this particular code change (/vxworks6/osnetwork.c, line 313)

[skliper]

Trac comment by sstrege on 2015-11-18 15:21:31:

Correction to comment 5 above, this was agreed to at the 11/17/15 CCB

[skliper]

Trac comment by glimes on 2015-11-20 16:13:33:

now part of ic-2015-11-20

[skliper]

Trac comment by jphickey on 2015-11-20 17:05:05:

To re-iterate the comment above - the VOIDFUNCPTR cast absolutely needs to be double-checked by someone with access to the VxWorks API documentation for its timer_connect() call, before we accept this into development branch.

If timer_connect truly wants a function with a void return/void argument and OSAL is passing a function that has a void return/int argument, then that is a real bona-fide bug and a cast will only shut off the warning about it. If that is the case (as it appears to be) then we need a fix for the bug and not to cast away the warning about it.

[skliper]

Trac comment by jphickey on 2015-11-20 17:27:03:

Backed this out of the "ic-2015-11-20" merge pending verification of VOIDFUNCPTR cast.

[skliper]

Trac comment by jphickey on 2015-11-20 17:28:18:

IIRC it was Steve who was going to check the VxWorks doc on this -- If I got that wrong please pass it to whoever was going to double check this.

[skliper]

Trac comment by sduran on 2015-11-23 12:12:11:

From the VxWorks 6.7 man page:

 int timer_connect
     (
     timer_t     timerid,  /* timer ID      */
     VOIDFUNCPTR routine,  /* user routine  */
     int         arg       /* user argument */
     )

where "routine" is the signal handler, defined on line 149 in ostime.c
void OS_TimerSignalHandler(int host_timer_id)

ic-ccb-review has the handler declared as void, but trac-22 had changed this to static void, due to the MISRA rule that wants all local functions declared as static.
Recommend changing void back to static void as in trac-22.

The cast to VOIDFUNCPTR appears unecessary. A compiler warning is not generated without it. MISRA, I'm sure does not require a cast like this. Not sure why it was added.

I did note however, that the possible_tid type is mismatched from the expected type of timer_connect. I believe this has always been the case in this code. Not sure if we want to do anything about that now. possible_tid is declared as uint32, the the argument timer_id (which gets the value of possible_tid before return) is declared uint32, however the expected argument type for timer_connect is int.

[skliper]

Trac comment by jphickey on 2015-11-23 23:19:51:

In this case, we may actually have a mismatch in the VxWorks coverage stubs. This defines the VOIDFUNCPTR type as:
{{{
typedef void (*VCS_VOIDFUNCPTR) (void);
}}}

i.e. a function taking no arguments, returning void.

However, it looks from this timer_connect API that a VOIDFUNCPTR here is intended to take a single int argument, perhaps this needs to be:
{{{
typedef void (*VCS_VOIDFUNCPTR) (int);
}}}

With respect to this function being static -- the static keyword is included in this changeset, but not in ic-ccb-review as we were awaiting the final word on this type cast before merging it.

I also agree that the type for the host_timerid field of the internal structure should be timer_t as opposed to uint32 which it is right now. This should probably be separate ticket.

[skliper]

Trac comment by jphickey on 2015-11-24 00:07:25:

Replying to [comment:11 sduran]:

The cast to VOIDFUNCPTR appears unecessary. A compiler warning is not generated without it. MISRA, I'm sure does not require a cast like this. Not sure why it was added.

With this I have added a new commit [changeset:94f1adc] that reverts the "VOIDFUNCPTR" cast only.

This timer_connect() code still looks suspicious to me -- not sure how much this code has actually been tested.... Assuming that VOIDFUNCPTR does indeed take an int argument, this is passing possible_tid as the user argument and then inside OS_TimerSignalHandler it is comparing this to host_timerid in the table. I wouldn't expect these to match, except by luck if the OS hands out an ID of "0" first just like OSAL does.

At any rate, someone more familiar with VxWorks would have to look at that, and this ticket will not change the behavior.

I have merged this to "ic-ccb-review" and I think this is good to go.

[skliper]

Trac comment by glimes on 2015-11-24 13:47:52:

''TLDR: casts are what you use on broken code, and this VxWorks API is broken,
so applying a cast here is unfortunate but probably appropriate :(''

Notes on VOIDFUNCPTR ...

This was a bit confusing, as the code as it stands follows the common
idiom and can be expected to run correctly.

HOWEVER, static analysis tools and some compilers can be really picky
about category errors in type matching. So here is the story.

The actual implementation in VxWorks eventually needs a pointer to a
function that takes an integer and returns void; and our callback
function is in fact matching that signature.

The issue arises that VxWorks defines the API in its time.h file
as taking a VOIDFUNCPTR as its second argument, and in host.h
it defines VOIDFUNCPTR as typedef void (*VOIDFUNCPTR)(); ...

Picky tools will decide that the type of a function taking (int) does
not match a () (''arguments unspecified'') declaration. This makes
some sense when doing very strict checking, as passing over this
without a warning means I might be passing a function taking (double)
where you are expecting a function taking (char *foo, int bar) ...

Note that strict analysis may also warn about VOIDFUNCPTR itself,
as it defines a function type but is not itself a valid function
prototype indicating parameter types.

[skliper]

Trac comment by glimes on 2015-12-09 10:56:06:

Commit [changeset:1f9e588] containing the bulk of the work was included via [changeset:57cd4e8a] into merge [changeset:961b061] which is now the development branch.

[skliper]

Trac comment by glimes on 2015-12-09 11:13:56:

Merge of #136 #138 #139 #141 and #142 (2015-12-01) is now development.