Skip to content

Conversation

@zimri-leisher
Copy link
Collaborator

Related Issue(s) #3817
Has Unit Tests (y/n) y
Documentation Included (y/n) n

Change Description

Adds floating point <, >, <=, >=, == and != operators.

Rationale

Users of Fpy need to be able to compare floating point values, not just integers.

@zimri-leisher zimri-leisher requested a review from LeStarch June 27, 2025 02:46

//! Internal interface handler for directive_setSerReg
Signal FpySequencer::setSerReg_directiveHandler(const FpySequencer_SetSerRegDirective& directive, DirectiveError& error) {
Signal FpySequencer::setSerReg_directiveHandler(const FpySequencer_SetSerRegDirective& directive,

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
}

Signal FpySequencer::deserSerReg_directiveHandler(const FpySequencer_DeserSerRegDirective& directive, DirectiveError& error) {
Signal FpySequencer::deserSerReg_directiveHandler(const FpySequencer_DeserSerRegDirective& directive,

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.

Signal FpySequencer::binaryCmp_directiveHandler(const FpySequencer_BinaryCmpDirective& directive, DirectiveError& error) {

Signal FpySequencer::binaryCmp_directiveHandler(const FpySequencer_BinaryCmpDirective& directive,

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.

Signal FpySequencer::binaryCmp_directiveHandler(const FpySequencer_BinaryCmpDirective& directive, DirectiveError& error) {

Signal FpySequencer::binaryCmp_directiveHandler(const FpySequencer_BinaryCmpDirective& directive,

Check notice

Code scanning / CodeQL

Function too long Note

binaryCmp_directiveHandler has too many lines (99, while 60 are allowed).
@zimri-leisher
Copy link
Collaborator Author

@LeStarch looks like it's failing CI due to some nondeterminism, can you take a look?

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.

In general it looks good, but I did wonder about some things.

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@zimri-leisher
Copy link
Collaborator Author

@LeStarch this PR is WIP, missing a couple UTs

@zimri-leisher
Copy link
Collaborator Author

@LeStarch this PR is ready for review again, I think it's good to go as well. code is much cleaner now

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.

Much, MUCH easier to review. Thanks!

@LeStarch LeStarch merged commit e3376dc into nasa:devel Jul 3, 2025
51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants