JSC: Replace "UNINITIALIZED" macro with "OS_UNINITIALIZED"

[skliper]

Originally part of trac #45 and split off for review purposes. Only affects vxworks6.

[skliper]

Imported from trac issue 113. Created by jphickey on 2015-11-06T16:50:58, last modified: 2016-03-07T15:55:03

[skliper]

Trac comment by jphickey on 2015-11-09 10:59:48:

Commit [changeset:2c54b11] is available for CCB review. Branch trac-113-jsc-uninitialized-macro-change

[skliper]

Trac comment by jphickey on 2015-11-09 11:06:46:

I have reviewed the changeset and it is unclear to me what this is trying to accomplish. Is there a bona-fide namespace clash with the "UNINITIALIZED" macro name?

Justification issues aside, I do not recommend accepting this in current form anyway.

My objections are:

• Putting OS_UNINITIALIZED in the public osapi.h header file -- the comment says it's not part of the OSAL API, so why is it in this file? This is not the correct place to put private (implementation-specific) macro definitions.

• The specific value of OS_UNINITIALIZED is 0, which is not discernible from //VALID// object identifiers of 0. If anything this should be 0xFFFFFFFF, but that might break other things, so we should not change it. As is, there really is no benefit to making this value a macro, so my recommendation (if there is a name conflict) is to lose the macro entirely and just use a literal 0.

[skliper]

Trac comment by sduran on 2015-11-10 12:37:30:

This change was put in place because UNINITIALIZED was defined multiple times in different osal .c files. So it was put in one place in a h file, and to be consistent with OSAL naming, named OS_ UNINITIALIZED. The value of "0" was what UNINITIALIZED was originally defined to. I think this is better than hardcoding 0's.

[skliper]

Trac comment by sstrege on 2015-11-16 17:37:03:

I agree, use of the macro is better than hardcoding 0's. I verified that each definition of UNINITIALIZED was set to the value of "0". This set of changes is not changing the code base in anyway.

The comment "Items below here are internal OSAL-use definitions and are not part of the OSAL API" is very confusing. I recommend updating the comment (adding a warning that the definition may not discernible from VALID object identifiers of 0 and should be used with caution) or moving the OS_UNINITIALIZED macro definition back into src/os/vxworks6/osapi.c.

[skliper]

Trac comment by jphickey on 2015-11-17 10:41:29:

Replying to [comment:4 sstrege]:

The comment "Items below here are internal OSAL-use definitions and are not part of the OSAL API" is very confusing. I recommend updating the comment (adding a warning that the definition may not discernible from VALID object identifiers of 0 and should be used with caution) or moving the OS_UNINITIALIZED macro definition back into src/os/vxworks6/osapi.c.

This was my main objection as well. If it is not part of the official/public OSAL API then it does not belong in the public header files, at all. This should be fixed before CCB accepts this.

This OS_UNINITIALIZED value is specific to the VxWorks6 implementation for internal use only, and should only be defined in the internal implementation, not in the public API. Making this value public serves no purpose - I cannot think of any place that application code could feasibly reference this and have it //NOT// be a bug.

My recommendation is to put the #define back in the C files (osapi.c and ostimer.c).

If the issue is that the same macro name appears in two places, then call the one in osapi.c VXWORKS_OSAPI_UNINITIALIZED and the one in ostimer.c VXWORKS_OSTIMER_UNINITIALIZED.

There is no need to ensure that they both have the same value. As I said, nothing ever compares to this value (because 0 is valid, it cannot ever do that, if it did it is a bug). I would wager you could put any random value in here, even a different one for every time the UNINITIALIZED macro is used and it wouldn't affect the runtime operation.

[skliper]

Trac comment by sduran on 2015-11-24 13:07:25:

Addressed the handling of UNINITIALIZED in commit [changeset:0687fbd]

[skliper]

Trac comment by glimes on 2015-12-09 11:02:34:

Commits [changeset:2c54b116] and [changeset:0687fbd] containing this change included via [changeset:2ce04e5] 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.

[skliper]

Trac comment by jphickey on 2016-02-18 10:43:48:

Upon review of the current 4.2 development branch it seems that the addition of OS_UNINITIALIZED to the public osapi.h file is still there.

Re-opening so this can be removed before 4.2 is finalized.

[skliper]

Trac comment by sstrege on 2016-03-07 15:28:57:

commit [changeset:0d90392] removes definition of OS_UNINITIALIZED from the public osapi.h file