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.

Sign up for GitHub

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

Jump to bottom

Fix #410, breakup includes #675

Merged
merged 1 commit into from
Dec 14, 2020
Merged

Fix #410, breakup includes #675

merged 1 commit into from
Dec 14, 2020

Conversation

jphickey
Copy link
Contributor

@jphickey jphickey commented Dec 9, 2020 •
edited by skliper
Loading

Describe the contribution
Break up osapi-os-*.h files into units which correspond to the implementation units. The old header file names still exist for compatibility.

Fix #410

Testing performed
Build and run CFE and unit tests.

Expected behavior changes
None - just header refactoring, no actual change.

System(s) tested on
Ubuntu 20.04
RTEMS 4.11

Contributor Info - All information REQUIRED for consideration of pull request
Joseph Hickey, Vantage Systems, Inc.

@jphickey jphickey added the CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) label Dec 9, 2020
@jphickey
Copy link
Contributor Author

jphickey commented Dec 9, 2020

Only marked as draft because it requires a baseline which is not yet in main - will rebase once next baseline is settled.

skliper
skliper reviewed Dec 9, 2020
View reviewed changes
src/bsp/shared/src/bsp_default_symtab.c Outdated Show resolved Hide resolved
src/os/inc/osapi-common.h Outdated Show resolved Hide resolved
@skliper skliper linked an issue Dec 9, 2020 that may be closed by this pull request
Break up osapi-os-core.h into more focused includes #410
Closed
@astrogeco astrogeco added CCB-20201209 and removed CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) labels Dec 9, 2020
@astrogeco
Copy link
Contributor

CCB 2020-12-09 APPROVED

  • remove "INCLUDE" from the include guard
  • swap <> for "" in includes since they are reserved for system libraries
  • remove jpfix ifdef

@jphickey
Fix nasa#410, breakup OSAL includes
f0bd42e 
Includes compatibility wrappers for the old header names.

Refactors inclusions in all OSAL source files to only include
the components they actually use/refer to.

Refactor all inclusion guards to follow the same general
format (uppercase filename, no leading/trailing underscore)
@jphickey jphickey marked this pull request as ready for review December 10, 2020 20:08
@jphickey
Copy link
Contributor Author

Rebased to "main" and made all requested changes.

Note I used a regex script to replace all the include guards to match the requested pattern, as well as to use quoted filenames instead of bracketed filenames in #include directives. This ended up actually catching a lot of other improperly formatted guards across other parts of OSAL - not just the files originally changed. So as a result the number of files affected here has increased. But it should accomplish the goal of making all the include guards consistent.

@skliper
Copy link
Contributor

skliper commented Dec 14, 2020

@astrogeco can we move this one forward? It touches everything so the quicker we can get it in the less we are going to have to deal with conflicts.

astrogeco
astrogeco reviewed Dec 14, 2020
View reviewed changes
src/os/inc/osapi-os-loader.h
@@ -30,207 +30,18 @@
#ifndef _osapi_loader_
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to fix this include guard even though the file is deprecated?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@astrogeco can we move this one forward? It touches everything so the quicker we can get it in the less we are going to have to deal with conflicts.

I'll merge it now and we can revisit my commit in the IC as a hotfix if we want to fix include guard styles for deprecated code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW - I had skipped updating these files because they were deprecated... will probably remove in a future commit with the rest of the deprecated stuff in this cycle.

@astrogeco astrogeco merged commit 8dd7415 into nasa:integration-candidate Dec 14, 2020
astrogeco added a commit to nasa/cFS that referenced this pull request Dec 14, 2020
@astrogeco
Add nasa/osal#675
4ba2e22
This was referenced Dec 14, 2020
Merged
Merged
@jphickey jphickey deleted the fix-410-breakup-includes branch January 27, 2021 14:09
@skliper skliper added this to the 6.0.0 milestone Sep 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@astrogeco astrogeco astrogeco left review comments

@skliper skliper skliper approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Caelum
Development

Successfully merging this pull request may close these issues.

Break up osapi-os-core.h into more focused includes
3 participants
@jphickey @astrogeco @skliper