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

feat(HIS): add GOTO his metric #76

Open
wants to merge 1 commit into
base: wip/his-metrics
Choose a base branch
from

Conversation

AfonsoSantos96
Copy link
Member

@AfonsoSantos96 AfonsoSantos96 commented Nov 22, 2023

This PR introduces the feature of checking the existence of a goto statement in a function/file.
According to the HIS metrics, no goto statement is allowed
The qmcalc tool is used to check this metric.
This tool only outputs the existence of a goto expression in the file, so our tool output won't specify the function where the expression is used, only highlighting the file where the expression is used.


Edited by @danielRep :
To test with https://github.com/bao-project/bao-his-tests just run the command make ci METRIC=goto.

Checklist:

  • Tested using the bao-his-tests repo.
  • Compared and validated outputs to ECLAIR his metrics assessment on bao-his-tests repo
  • The changes generate no new warnings when building the project. If so, I have justified above.
  • I have run the CI checkers before submitting the PR to avoid unnecessary runs of the workflow.

danielRep
danielRep previously approved these changes Dec 18, 2023
Copy link
Member

@danielRep danielRep left a comment

Choose a reason for hiding this comment

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

Tested and it LGTM.

@danielRep
Copy link
Member

danielRep commented Feb 16, 2024

Updated with the modifications introduced in #68 and changed the output format.

@miguelafsilva5 @josecm
Use the testing repo (https://github.com/bao-project/bao-his-tests) has a dummy testbed for each individual metric verified by the HIS checker.
You only need to clone the repo, checkout the ci to this branch, and then make ci METRIC=goto.

@danielRep danielRep removed their assignment Feb 16, 2024
@danielRep danielRep force-pushed the feat/his_goto branch 2 times, most recently from 3805722 to a7e1eb5 Compare February 16, 2024 14:05
@josecm josecm self-assigned this Feb 16, 2024
@danielRep danielRep force-pushed the wip/his-metrics branch 3 times, most recently from e298879 to 27fec24 Compare February 27, 2024 11:28
@danielRep danielRep force-pushed the feat/his_goto branch 3 times, most recently from 59c0a48 to 4b66c69 Compare February 27, 2024 14:17
@danielRep danielRep force-pushed the feat/his_goto branch 2 times, most recently from 6635b68 to 929ee95 Compare February 27, 2024 16:44
@miguelafsilva5
Copy link
Member

Tested using the repo provided by @danielRep. From my observations, it seems to be working as intended.
The only corner case that might have mislead results that I found was hiding the goto statement in a macro (e.g #define HIDING goto). Then using that macro in other files is not detected as a goto statement. The checker only points to the original file where it is define and not where it is used. Furthermore, if said macro is not used, the checker also fails saying that a goto statement is present.
However, since the threshold is 0, it is should not be a problem.

@danielRep danielRep changed the title feat(HIS): add goto statement checker feat(HIS): add GOTO his metric Feb 29, 2024
@danielRep
Copy link
Member

Tested using the repo provided by @danielRep. From my observations, it seems to be working as intended. The only corner case that might have mislead results that I found was hiding the goto statement in a macro (e.g #define HIDING goto). Then using that macro in other files is not detected as a goto statement. The checker only points to the original file where it is define and not where it is used. Furthermore, if said macro is not used, the checker also fails saying that a goto statement is present. However, since the threshold is 0, it is should not be a problem.

That's a corner case that the tool would not detect. For such corner cases we would need to have a specific implementation.

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.

4 participants