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

Fix #1448, Simplify and clarify EVS_AddLog logic #2309

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

thnkslprpt
Copy link
Contributor

Checklist

Describe the contribution

Testing performed
GitHub CI actions all passing successfully.

Would be good to add functional tests for this in the future - I noticed that changing the second block to just a simple else also passes all the coverage tests.

Expected behavior changes
No change.

Contributor Info
Avi Weiss @thnkslprpt

@thnkslprpt
Copy link
Contributor Author

I can also update to the more terse version if there is a general desire to move in that direction as updates are made:

if (CFE_EVS_Global.EVS_LogPtr->LogFullFlag)
...
if ((!CFE_EVS_Global.EVS_LogPtr->LogFullFlag) ||
...

Side note - LogFullFlag is actually not a bool type - although it's treated as such.

Works fine obviously, but I think I'll open a new issue about it - just checking if there are any other similar cases first (ints being treated as bools). Probably left over from earlier times before bool was commonly used.

@chillfig chillfig self-requested a review November 7, 2024 19:01
@chillfig chillfig added CCB:Ready Ready for discussion at the Configuration Control Board (CCB) enhancement labels Nov 7, 2024
@dzbaker dzbaker added CCB:Approved Indicates code review and approval by community CCB and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CCB:Approved Indicates code review and approval by community CCB enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify LogOverflowCounter increment logic
4 participants