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 STMT his metric #74

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 9, 2023

This PR introduces the feature of checking the number of statements within each function.
Each function must only have a maximum of 50 statements.
The pmccabe tool is used to check this metric.

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

Checklist:

  • 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
Copy link
Member

danielRep commented Nov 10, 2023

Tested and it LGTM.

danielRep
danielRep previously approved these changes Dec 18, 2023
@josecm josecm self-assigned this Feb 16, 2024
@AfonsoSantos96 AfonsoSantos96 dismissed danielRep’s stale review February 25, 2024 02:15

The merge-base changed after approval.

@danielRep danielRep force-pushed the wip/his-metrics branch 5 times, most recently from c1452fe to 4a2edbb Compare February 27, 2024 17:00
@danielRep danielRep marked this pull request as draft February 29, 2024 13:36
@danielRep danielRep changed the title feat(HIS): added checker of number of statements to script feat(HIS): add STMT his metric Feb 29, 2024
Signed-off-by: Afonso Santos <afomms@gmail.com>
@danielRep
Copy link
Member

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

@danielRep danielRep marked this pull request as ready for review February 29, 2024 16:21
@danielRep danielRep removed their assignment Feb 29, 2024
@miguelafsilva5
Copy link
Member

I've tested this PR and it looks ok to me. However, I found that the tool counts any macro as a single statement, regardless of its contents. A function-like macro easily "hide" several statements from this tool.
Also, while correct, inline assembly can "hide" statements like function calls, and the tool counts it as a single statement.

@danielRep
Copy link
Member

danielRep commented Mar 25, 2024

I've tested this PR and it looks ok to me. However, I found that the tool counts any macro as a single statement, regardless of its contents. A function-like macro easily "hide" several statements from this tool.

Yes, that's a big limitation from pmccabe. From its manual:

Parsing
pmccabe ignores all cpp preprocessor directives - calculating the complexity of the
appearance of the code rather than the complexity after the preprocessor mangles the code.
This is especially important since simple things like getchar

However, I didn't found any other tool that outputs number of statements per function as pmccabe does. I'll keep looking.

Also, while correct, inline assembly can "hide" statements like function calls, and the tool counts it as a single statement.

Didn't get this second point, could you share an example?

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.

None yet

4 participants