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

Address comments from #2485 #2542

Merged
merged 10 commits into from
Mar 1, 2024
Merged

Address comments from #2485 #2542

merged 10 commits into from
Mar 1, 2024

Conversation

bocchino
Copy link
Collaborator

Closes #2530.

github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 22, 2024
View reviewed changes
Fw/Dp/DpContainer.cpp Dismissed Show dismissed Hide dismissed
Fw/Dp/DpContainer.cpp Dismissed Show dismissed Hide dismissed
Fw/Dp/DpContainer.cpp Dismissed Show dismissed Hide dismissed
Fw/Dp/DpContainer.cpp Dismissed Show dismissed Hide dismissed
Fw/Dp/DpContainer.cpp Dismissed Show dismissed Hide dismissed
Fw/Dp/DpContainer.cpp Dismissed Show dismissed Hide dismissed
Fw/Dp/DpContainer.cpp Dismissed Show dismissed Hide dismissed
Fw/Dp/DpContainer.cpp Dismissed Show dismissed Hide dismissed
Fw/Dp/DpContainer.cpp Dismissed Show dismissed Hide dismissed
Fw/Dp/DpContainer.cpp Dismissed Show dismissed Hide dismissed
github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 22, 2024
View reviewed changes
Fw/Dp/DpContainer.cpp Dismissed Show dismissed Hide dismissed
Fw/Dp/DpContainer.cpp Dismissed Show dismissed Hide dismissed
Fw/Dp/DpContainer.cpp Dismissed Show dismissed Hide dismissed
Fw/Dp/DpContainer.cpp Dismissed Show dismissed Hide dismissed
Fw/Dp/DpContainer.cpp Dismissed Show dismissed Hide dismissed
Fw/Dp/DpContainer.cpp Dismissed Show dismissed Hide dismissed
Fw/Dp/DpContainer.cpp Dismissed Show dismissed Hide dismissed
Fw/Dp/DpContainer.cpp Dismissed Show dismissed Hide dismissed
Fw/Dp/DpContainer.cpp Fixed Show fixed Hide fixed
Fw/Dp/DpContainer.hpp Dismissed Show dismissed Hide dismissed
LeStarch
LeStarch approved these changes Feb 26, 2024
View reviewed changes
Copy link
Collaborator

@LeStarch LeStarch left a comment

Choose a reason for hiding this comment

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

I see the requested changes and tests there-in. Code looks good. QL checks all seem to be false-positives.

timcanham
timcanham requested changes Feb 27, 2024
View reviewed changes
Copy link
Collaborator

@timcanham timcanham left a comment

Choose a reason for hiding this comment

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

Just a couple more comments...

Fw/Dp/DpContainer.cpp Outdated Show resolved Hide resolved
Fw/Dp/DpContainer.cpp Outdated Show resolved Hide resolved
github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 27, 2024
View reviewed changes
Fw/Dp/DpContainer.cpp Dismissed Show dismissed Hide dismissed
@bocchino bocchino requested a review from timcanham February 27, 2024 01:23
timcanham
timcanham approved these changes Feb 27, 2024
View reviewed changes
Copy link
Collaborator

@timcanham timcanham left a comment

Choose a reason for hiding this comment

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

Good changes!

@LeStarch
Copy link
Collaborator

@bocchino I think this is ready. Would you triage the CI annotations?

@bocchino
Copy link
Collaborator Author

bocchino commented Feb 28, 2024 •
edited
Loading

Done.

@bocchino
Copy link
Collaborator Author

bocchino commented Feb 29, 2024 •
edited
Loading

There's one more fix I'd like to do on this branch before merging it. That's to merge in devel and replace FwSizeType with FwSizeStoreType in the data product header.

@bocchino
Copy link
Collaborator Author

The last commit is for CI checking. It needs one more change before it's ready for merge.

github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 29, 2024
View reviewed changes
Fw/Types/Serializable.cpp Dismissed Show dismissed Hide dismissed
github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 29, 2024
View reviewed changes
Fw/Dp/DpContainer.hpp Dismissed Show dismissed Hide dismissed
Fw/Types/Serializable.cpp Dismissed Show dismissed Hide dismissed
Fw/Types/Serializable.cpp Dismissed Show dismissed Hide dismissed
Fw/Types/Serializable.cpp Fixed Show fixed Hide fixed
@bocchino
Copy link
Collaborator Author

I reverted the temporary change.

@bocchino
Copy link
Collaborator Author

My changes are done, once CI passes.

@bocchino
Copy link
Collaborator Author

With the change in FwSizeType to I64, I noticed some new compiler warnings in the FPP compilation checks, which have -Wconversion enabled. It looks like we are not consistent in representing size values. Many times we use U32 as the size type, e.g., in the size of a hashed value or an Fw::Buffer. We should improve the consistency. In the mean time, I added casts to suppress the warnings.

@LeStarch LeStarch merged commit b5156ad into nasa:devel Mar 1, 2024
34 checks passed
@bocchino bocchino deleted the issue-2530-dp-framework branch March 5, 2024 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LeStarch LeStarch LeStarch approved these changes

@timcanham timcanham timcanham approved these changes

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

Successfully merging this pull request may close these issues.

Address Follow-Up to #2485
3 participants
@bocchino @LeStarch @timcanham