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

Pointer arguments to functions that are input only should be declared "const" #59

Closed
skliper opened this issue Sep 30, 2019 · 11 comments
Closed
Milestone

Comments

@skliper
Copy link
Contributor

skliper commented Sep 30, 2019

Marking the pointer parameter as "const", particularly for string arguments, allows them to be called using string literals or other data that is already constant.

This may make a substantial difference on some targets where the executable can actually be linked to put the read-only data section in ROM rather than RAM. However, in order to do this properly/safely the code must treat this as read-only data. By declaring it "const" the compiler will flag any writes to it.

Changing the prototypes should not affect current usage.

@skliper skliper added this to the 6.5.0 milestone Sep 30, 2019
@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Imported from trac issue 28. Created by jphickey on 2015-01-22T09:48:19, last modified: 2019-03-05T14:57:55

@skliper skliper self-assigned this Sep 30, 2019
@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jphickey on 2015-01-26 16:24:11:

Branch "trac-28-const_pointer_args" pushed -- commit [changeset:bed05ea]

This adds a "const" designation to the function prototypes where applicable. This set may not be 100% complete, but it gets all the low hanging fruit.

This changeset also rolls in the fix for #40 and #52

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jphickey on 2015-03-13 16:02:06:

DEPENDENCY NOTE: using this change set also requires taking PSP trac 15:

[https://babelfish.arc.nasa.gov/trac/cfs_psp/ticket/15]

This fixes the PSP calls to adhere to the modified prototypes on the CFE side.

There is also a separate merge commit [changeset:2bd5438] that is also required when merging this with the fix for trac #32. These are pushed separately since there has not been a new baseline (yet) that already includes trac 1.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jphickey on 2015-04-06 11:51:56:

This is ready for review/merge

The merge changeset for the two parts of this fix can be viewed in [changeset:29c1ec9].

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by sstrege on 2015-04-06 13:18:55:

Concur with changes involving the addition of the "const" keyword.

The diff did show some other changes that are unrelated to this ticket:

cfe_time_utils.h - copyright character changed?

cfe_tbl_task_cmds.c - changed PSP_MemCpy to strncpy - I'm OK with this change

es_UT.c - removed typecast from function prototypes?

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jphickey on 2015-04-06 13:59:26:

My comments to the comments:

  • cfe_time_utils.h copyright character - This was unintentional and appears to be due to Eclipse "fixing" encoding issues with utf-8, as the original was neither utf-8 nor ascii. Warrants another discussion on whether we should make all files UTF-8 or stick to straight ASCII? If the latter, we need to remove all special characters and replace it with e.g. (c). But this is a separate issue. I can revert this back using a different tool but this will be invalid utf-8 again.

  • es_UT.c - After fixing the prototypes, the typecasting on these calls becomes unnecessary and actually wrong - the compiler would complain about a type mismatch here //due to// the typecast. Now the simple way is also the right way.

  • cfe_tbl_task_cmds.c - This change also got inadvertently pulled into this change set. (FYI I am backporting/isolating all these changes from my other work tree). I can put this in a separate ticket if desired - let me know!

However, the change from memcpy() to strncpy() is absolutely necessary here - the former will read past the end of the source string if it is less than CFE_TBL_MAX_FULL_NAME_LEN characters.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by acudmore on 2015-04-07 12:38:18:

Concur.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by glimes on 2015-04-07 13:02:50:

Tested changesets [changeset:bed05ea51] [changeset:29c1ec922] as part of the ic-2015-03-10 merge.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by glimes on 2015-04-13 15:14:21:

Part of integration candidate 2015-03-10,
committed to cFS CFE Development branch on 2015-04-10
as part of merge [changeset:7d6f6d0].

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by glimes on 2016-02-16 13:16:45:

Susie confirmed these tickets have been approved for CFE 6.5

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jhageman on 2019-03-05 14:57:55:

Milestone renamed

@skliper skliper closed this as completed Sep 30, 2019
@skliper skliper removed their assignment Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant