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

Clean up "extern" declarations at the top of C files #39

Closed
skliper opened this issue Sep 30, 2019 · 13 comments
Closed

Clean up "extern" declarations at the top of C files #39

skliper opened this issue Sep 30, 2019 · 13 comments
Labels
Milestone

Comments

@skliper
Copy link
Contributor

skliper commented Sep 30, 2019

In the CFE core apps, many files reference functions and data structures defined in other files. However, the function prototypes or "extern" declarations are not in common header files, but simply put at the top of the C file that uses it.

While this does build, it defeats the type checking done by the compiler.

It is far from ideal because if the real variable type or function prototype ever changes, the linker will still happily link it together even though they might be completely incompatible (or worse, incompatible in a really subtle way).

The only reason to NOT put a declaration in a header file is if it should not be called or referenced by CFS apps, but this can be solved by creating a private CFE core shared header file.

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

skliper commented Sep 30, 2019

Imported from trac issue 8. Created by jphickey on 2014-12-30T19:34:31, last modified: 2019-03-05T14:57:55

@skliper skliper added the bug label Sep 30, 2019
@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jphickey on 2015-01-26 14:00:03:

Pushed branch "trac-8-extern_decl_cleanups" to address this

Commit [changeset:62ec03db]

Ensure that all functions/variables that are referenced from a ".c" file other than the file containing the implementation/instantiation are properly prototyped in a header file.

The following rule was used to determine where the prototype/declaration should reside:

  • For symbols that are only referenced within the same core app (e.g. ES calling another function within ES), place the declaration in a header file within the same core app. This also applies to symbols referenced from the unit test of the same app.
  • For symbols that are referenced by other CFE core applications but not outside CFE core, place the declaration in a newly introduced "private" directory under the existing "inc" directory. Note that placing it here rather than in a separate directory such as "private_inc" allows the existing makefiles to find the new files without modifying include search paths.
  • For symbols that are referenced outside CFE core, place the declaration in the public "inc" directory under the corresponding module.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jphickey on 2015-01-26 14:05:13:

Important note regarding merging of the basic fix above:

An additional branch "merge-8+1-cmake_plus_refactor" is also pushed to reflect the merge between this fix and trac #32 (new build/cmake files).

Commit [changeset:ddecc89b]

This basically removes some extra items that had been required to make the build work without the trac 8 change in place. They become unnecessary after trac 8 is merged in.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by glimes on 2015-02-11 11:50:53:

Current version is [changeset:642acb9] plus [changeset:ddecc89b] merged in [changeset:e7a6636]

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jphickey on 2015-04-06 11:17:06:

This is ready for review/merge

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by acudmore on 2015-04-06 14:24:18:

I think the reasoning is good, recommend accept.

Do you think anyone will confuse the inc/private files with the subsystem private headers? ( cfe_sb_priv.h? )

Could the private files implemented with a name and excluded from apps using a #ifdef guard?
( keeping them more consistent with the cfe_*_priv.h files )

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jphickey on 2015-04-06 14:37:29:

With respect to an #ifdef guard --

Actually my original version of this fix had this, the make script would define the CFE_CORE macro when compiling the core apps, and the private include files started in an #ifndef that would error out if someone tried to include them from somewhere else.

The only problem with this approach is that it broke the non-CMake build using the old makefiles, as these did not define the CFE_CORE macro. However they could probably be fixed to do so.

The other option is to put these into a directory at the same level as the "inc" (such as "private_inc") but this also requires a fix to both sets of makefiles to add another -I to the compiler command, and I think there are too many of these already.

At the end of the day I decided it was best to just leave well enough alone. With the way it is now, if someone includes this in an app they actually have to write {{{ #include "private/something.h" }}} which should set off some red flags that this is possibly a bad idea. If the code happens to work anyway, then that is OK, but they will be using it "at their own risk"....

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jwilmot on 2015-04-06 15:12:34:

Concur. Need to apply this solution to SBN which needs private headers.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by glimes on 2015-04-07 12:50:15:

Tested changesets [changeset:642acb9] [changeset:ddecc89] [changeset:e7a6636] 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-07 13:46:01:

Will retain these changesets in the merge; a new trac ticket will be opened
to track any further incremental improvemtns.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by glimes on 2015-04-13 15:13:24:

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

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

No branches or pull requests

1 participant