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

os: avoid using result of an assignment operator #72227

Merged

Conversation

DeHess
Copy link
Collaborator

@DeHess DeHess commented May 2, 2024

Fix coding guideline MISRA C:2012 Rule 13.4 in os:

The result of an assignment operator should not be used.

This PR is part of the enhancement issue #48002 which port the coding guideline fixes done by BUGSENG on the https://github.com/zephyrproject-rtos/zephyr/tree/v2.7-auditable-branch back to main

The commit in this PR is a subset of the original auditable-branch commit:
64336f4

@DeHess
Copy link
Collaborator Author

DeHess commented May 17, 2024

Converted to for-loops, since this was preferred in other prs.

@DeHess DeHess marked this pull request as draft May 17, 2024 08:16
- avoid to use assignment expression value

Signed-off-by: Hess Nathan <nhess@baumer.com>
@DeHess DeHess force-pushed the misra_rule_13_4_auditable_to_main_os branch from aaef295 to 58f37bc Compare May 17, 2024 08:56
@DeHess DeHess marked this pull request as ready for review May 17, 2024 10:47
@dcpleung
Copy link
Member

Hm... I think the for loop is even more confusing as you have two assignments there.

@DeHess
Copy link
Collaborator Author

DeHess commented May 22, 2024

@dcpleung

Hm... I think the for loop is even more confusing as you have two assignments there.

This PR does not necessarily address readability. It addresses the violation of misra coding guideline 13.4:

The result of an assignment operator should not be used

The previous while loop violated this rule, and the for loop I propose to use is the least terrible way to deal with it IMO.

Currently upstream:

while (((fract <<= 1) & BIT_63) == 0) {
	expo--;	
}

Proposed by Bugseng:

for (;;) {
	fract <<= 1;
	if ((fract & BIT_63) != 0) {
		break;
	}
	expo--;
}

Proposed by me:

for (fract <<= 1; (fract & BIT_63) == 0; fract <<= 1) {
	expo--;
}

Here is a discussion about the same topic regarding the same issue in the kernel part of the project:
#72226

@andyross
Copy link
Contributor

Yeah, the for() construction was proposed by me in a different PR, because while it's not as clean as the "while(iter = next())" idiom it at least puts the loop invariant at the top of the loop. I really hate the inline break, which is a terrible readability and maintenance cost to pay just for a pedantic rule. The for() duplicates an expression but at least reads naturally.

@andyross
Copy link
Contributor

And is Bugseng actually recommending the infinite loop construct? Yikes.

@dcpleung
Copy link
Member

Just curious, had this been run through the tool again to see if it complains?

@DeHess
Copy link
Collaborator Author

DeHess commented May 24, 2024

@andyross

And is Bugseng actually recommending the infinite loop construct? Yikes.

Yes. For most of these PRs I merely port forward what's on the auditable branch that they left us with.

@DeHess
Copy link
Collaborator Author

DeHess commented May 24, 2024

@dcpleung

Just curious, had this been run through the tool again to see if it complains?

No, the Tool Bugseng used (Eclair) is proprietary. So unfortunately I don't have access to it and can't check. That being said, my proposed construct is a standard for loop. If it violated the misra guideline, all for loops would.

There are currently efforts underway by @simhein and others to incorporate the Eclair Tool into the CI Pipeline in some way or other. My PRs are supposed to pave the way, so that once we use the tool to go over the entire project again, most violations will already be fixed.

@nashif nashif added the area: Coding Guidelines Coding guidelines and style label May 27, 2024
@nashif nashif merged commit fe06ffb into zephyrproject-rtos:main Jun 15, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants