Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide separate type for each resource category #913

Closed
jphickey opened this issue Sep 24, 2020 · 5 comments · Fixed by #1136
Closed

Provide separate type for each resource category #913

jphickey opened this issue Sep 24, 2020 · 5 comments · Fixed by #1136
Assignees
Milestone

Comments

@jphickey
Copy link
Contributor

Is your feature request related to a problem? Please describe.
Pull #896 provided a generic typedef for resource identifiers. This commit stops using uint32 and makes a dedicated type, but the type is the same for all resource categories (apps, tasks, counters, etc).

Describe the solution you'd like
Per @CDKnightNASA comment here:

#896 (comment)

It would improve things further to provide a separate/unique typedef for each resource category.

Additional context
This will be implemented as a follow-on to the original change.

Requester Info
Joseph Hickey, Vantage Systems, Inc.

@jphickey
Copy link
Contributor Author

One aspect of this that needs special consideration before we move forward with it - there are several constants and operations that work on the generic CFE_ES_ResourceID_t:

CFE_ES_RESOURCEID_UNDEFINED / CFE_ES_RESOURCEID_RESERVED for initialization
CFE_ES_ResourceID_ToInteger() / CFE_ES_ResourceID_FromInteger for printf/logging
CFE_ES_ResourceID_Equal() / CFE_ES_ResourceID_IsDefined() for checking

With a distinct ID type per category - in order to be fully type correct and type safe - one would technically need to add a version of these functions for every category. At least 5 resource categories are likely to exist, maybe more. So that means instead of 2 constants and 4 utility operations we will potentially need 10 constants and 20 utility ops, which will only grow more.

Again, if this were C++ with its better type system this would be a no brainer. But in C without compiler-aware inheritance or templates it gets pretty ugly and no easy migration path. I think this needs to be considered.

@CDKnightNASA
Copy link
Contributor

With a distinct ID type per category - in order to be fully type correct and type safe - one would technically need to add a version of these functions for every category.

If they're all based on the same parent typedef (a-la #896 (comment) ), I believe the compiler will behave and users could use the generic CFE_ES_ResourceID_Equal() [etc]. Running gcc -std=c99 -pedantic -Wall -Wstrict-prototypes -Wwrite-strings -Wpointer-arith -Wcast-align -Werror test.c on the following gave me no issues:

#include <stdio.h>

typedef struct { int a; } foo;
typedef foo bar;
bar baz;
void testfoo(foo *a) { printf("%d\n", a->a); }

int main(int argc, char **argv) { baz.a = 1; testfoo(&baz); }

@jphickey
Copy link
Contributor Author

Yes, a typedef is an alias - so you can interchange any typedef'ed names and it will work without error.

My point is that you'll be limited to that typedef - the AppID/LibID/CountID etc types can never become unique types without breaking all the code that is consistent between all of them. So what is the objective of making them separately named typedefs, if they can never be unique types?

This will start to become more relevant as I move to fix the old issue #28 -- for this it is actually helpful to have a single local ID variable that can refer to an app or a lib. Really for any API that can work on apps or libs, it is helpful to have an ID which is not type specific but still runtime safe in this regard.

@astrogeco
Copy link
Contributor

astrogeco commented Sep 30, 2020

CCB 2020-09-30

  • The C type system creates challenges with the concept of inheritance
  • Types should be unique and not "interchangeable" to prevent misuse and user error
  • Each type needs to have checking functions and other utilities so we get exponential growth of stuff

@jphickey
Copy link
Contributor Author

After doing #985 it is probably worthwhile to do some typedefs here:

  • CFE_ES_ResourceID_t should be renamed to CFE_ResourceID_t as it can be used in other subsystems too (SB) and the ES designation could become confusing. Likewise the ToInteger/FromInteger conversions should drop the "ES" too.
  • Add a module-specific typedef for each resource: CFE_ES_AppId_t, CFE_ES_LibId_t, CFE_ES_CounterId_t etc. These need to be straight typedefs - not separate/protected types - so it can share the same macros and conversions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants