strawman for #888 - return code for cFE #889
Conversation
|
note also I did a bit of formatting cleanup (joining multi-line into single line when it fits) |
There was a problem hiding this comment.
Just a quick look through, some more comments inline.
But also note that in order to be really type-agnostic, one must offer an inline function (or macro) to test for any value, not just success. That's because conditionals like:
if (Status == CFE_ES_ERR_TASKID)
no longer work if using a type for which a numeric equality test isn't implemented. In C++ it would be easy to simply provide a == operator, but in C we have to provide an explicit IsEqual function.
| */ | ||
|
|
||
| typedef int32 CFE_Status_t; | ||
| typedef CFE_Status_t CFE_RC_t; |
| * Utility macros. | ||
| */ | ||
| #define CFE_ISSUCCESS(S) ((S) & CFE_SEVERITY_BITMASK != CFE_SEVERITY_ERROR) | ||
| #define CFE_ISINFO(S) ((S) & CFE_SEVERITY_BITMASK == CFE_SEVERITY_INFO) |
There was a problem hiding this comment.
can these be inline functions instead? These are just as fast but more type safe and don't have the same risks/issues as macros.
There was a problem hiding this comment.
Just as fast if inlining is enabled. If it's not, and these are "real" functions, there's a decent amount of overhead.
If they are declared inline, they should be static inline at least, to avoid GOT/PLT lookup and such.
|
And also, FWIW, I'm still strongly in favor of getting rid of the notion of multiple success variants as indicated in #483. Functions should have either SUCCESS -- i.e. it worked fully and completely, or it FAILED - nothing in between. Having multiple different values for failure is fine - as it can indicate why it failed - but all of them should mean the same thing which is that the function did nothing at all (it failed). There shouldn't be any cases of "half-worked" -- that means the API is too complex. Any current API that returns half-worked "sorta success" codes should be split up and simplified into units that can fit the model of all or nothing. |
CFE_TBL is a standout for using non-error returns....Feel free to peruse the .h and consider alternative models? I can file a ticket for a redesign of CFE_TBL to remove the "WARN" and "INFO" return statuses. |
| @@ -176,7 +177,7 @@ static inline uint32 CFE_ES_ResourceID_FromInteger(unsigned long Value) | |||
| * @return Execution status, see @ref CFEReturnCodes | |||
| * @retval #CFE_SUCCESS @copybrief CFE_SUCCESS | |||
| */ | |||
| int32 CFE_ES_AppID_ToIndex(uint32 AppID, uint32 *Idx); | |||
| CFE_RC_t CFE_ES_AppID_ToIndex(uint32 AppID, uint32 *Idx); | |||
There was a problem hiding this comment.
Can we use CFE_ReturnCode_t ? I like spelling out things as much as possible
There was a problem hiding this comment.
A lot of typing, and causes more line wrapping...I'm open though if that's the consensus.
There was a problem hiding this comment.
Ah, I use autocomplete on Atom. I assume this type would be used by app developers so I'd rather be explicit. If it's just internal to cFE then I feel slightly better about abbreviations
|
CCB 2020-09-16 Discussed benefits and problems with multiple types. Also talked about the problems with multiple "success" and "failure" return codes. We should always ask, what is the purpose of the code? How can it be used? |
FYI - see my synopsis here for the various functions that need attention: Probably could have a whole separate discussion on each function. |
|
One vote for the naming discussion, note that "RC" is a term/acronym used throughout event messages and documentation in cFE/fsw/cfe-core/src/inc/cfe_es_events.h Line 421 in 60917a2 |
|
I'm biased against acronyms and initialisms, especially after reading "Acronyms Seriuosly Suck". Could "RtrnCode" be a good compromise? |
I am NOT a fan of the vowel-removal approach -- code that does this I find really ugly and hard to read. I also have a slight preference for full words, despite the fact that it makes long lines, I think the result is overall more readable. I also use an editor with autocomplete so the typing isn't a strong concern. My preference is I'm also OK with: But please - lets steer clear of quasi-abbreviations like "RtrnCode" -- this looks more like a letter jumble. |
|
I vote CFE_Status_t for the same reasons @jphickey states. |
|
consensus is CFE_Status_t, will update PR prior to next CCB. |
|
Marked as duplicate of #919 (this PR was the strawman for discussion). |
closes #888 -- not like this strawman will close it, but just creating the linkage
Describe the contribution
This is a strawman for return code typedef for cFE.
Testing performed
code builds
Expected behavior changes
Returns for cFE functions now typedef'd in headers (.h), not in the source (.c).
Contributor Info - All information REQUIRED for consideration of pull request
[email protected]