checkpatch: ignore UNNECESSARY_ELSE #93624
Conversation
In order to allow the code to comply with MISRA 15.7, which states: "All else if constructs shall be terminated with an else statement" Signed-off-by: Luis Ubieda <luisf@croxel.com>
|
zephyrbot
added
area: Continuous Integration
area: Coding Guidelines
zephyrbot
requested review from
carlescufi,
fabiobaltieri,
jfischer-no,
kartben,
keith-zephyr,
nashif,
simhein and
stephanosio
Regardless of validity of ones opinion, is that what the MISRA-C 2012 standard says? Why exactly should checkpatch not follow the approved Zephyr coding Rule 89? |
I don't argue its usefulness. I'm just pointing out that leaving this check in place keeps any code from meeting MISRA 15.7 whenever there's an What should developers follow, then? |
I have no idea what you mean by "usefulness." Checkpatch is right twice. And you could write your code in a way that makes it comply with checkpatch and does not have if else branch. But any rule that forces you to write dumb code like } else {
/* Proceed to evaluate data-frame */
}is a dumb rule, and we should neither follow nor have dumb rules. If that is a problem, then it is time to get rid of the MISRA rules. Also, they are not documented in the coding guidelines, nor is there openly available documentation or justification for why these rules should be followed. That is not acceptable for a project that calls itself a community project.
Do not follow dumb rules. Follow the general style that has been used for many years. |
"All if else if constructs shall be terminated with an else statement" |
That is neither documentation nor justification to follow it. |
None of what you say above is acceptable for any project, community or not. You need to stop this hostility to coding guidelines and to contributors trying to follow them in code reviews. The community has picked those rules, if you do not like them, then please use the right channels and process to change them or make them better. |
|
Nothing prevent them from writing the code differently, even without any else-if branches. And rules should not force anyone to write dumb code, so checkpatch behavior must not change. |
|
also, simple googling... https://www.mathworks.com/help/bugfinder/ref/misrac2023rule15.7.html 
this is like add default to switch statements. |
kartben
added
the
Hotfix
jfischer-no
removed
the
Hotfix
I was just waiting for that. Look, that is nonsense. Read what I wrote again. The documentation is not publicly available. No one may quote from this closed documentation. "a limited number of copies..." What the hell? It is either publicly available, or all MISRA rules must be dropped.
Really? A default without any statements? That is dumb as well, we should not do that too. btw, we already had that nonsense a few years ago when some of the "safety compliance patches," which blindly followed this rule, were merged and introduced bugs in a few subsystems. Others had to fix the problem afterwards. I mentioned it a few times to the safety people and they clearly answered that these rules should not be followed blindly. |
if (((header->events & BMP581_EVENT_FIFO_WM) != 0) && (*fit >= header->fifo_count)) {
return -ENODATA;
} else if (((header->events & BMP581_EVENT_FIFO_WM) == 0) && (*fit != 0)) {
return -ENODATA;
} else {
/* Proceed to evaluate data-frame */
} do we really want the code base full of these? doesn't strike me as good, compact, readable code, I think we can do better (and no I'm not talking about adding more parenthesis). As for MISRA, eh, I don't know what to say, it seems like whoever wrote those rules is a bit insecure about first grade math, you know, operator priorities, go figure. |
|
@jfischer-no Your derogatory comments are completely unacceptable. I would like to (once again) remind you about our Code of Conduct and that actions will be taken so that this behavior changes. Thanks. |
There was a problem hiding this comment.
Sorry I'll stick one more -1, not necessarily against it but I think the specific case that triggered this is not a great example, and in fact that should be changed instead, left a comment there. This check served the project well for a long time, we should keep it.
I agree that adding The MISRA C Rule 15.7 is "required" (i.e. not "mandatory") meaning deviations are allowed with a justification. But, as seen, it often results in extraneous/unnecessary code, and documenting the "justification" for all these cases is not going to scale. We probably should look into downgrading some MISRA C rule severity to "advisory" for our purposes with general justification ... p.s. speaking of which, there are so many |
You are making it about MISRA and having those rules be public. We decided to use MISRA as the source for our own coding guidelines, we are not trying to comply with MISRA, but MISRA, whether you like it or not, is the most complete coding guideline out there. Going back to the rule that conflicts with checkpatch, there is enough information out there already explaining it and in details and the same rule is also part of other guidelines. The rule when it was introduced, was reviewed, discussed and approved. Many rules were not adopted for various reasons. So, if this is about the rule and why we have it, start here: https://sonarcloud.io/organizations/zephyrproject-rtos/rules?languages=c&open=c%3AS126&q=else&tab=more_info We probably need to add references to those rules where they can be found (i.e. CERT and such) and stop making it about MISRA and it being public or not. Let's not make this about the PR and code in that PR that triggered this change to checkpatch, the code itself can be done in diffferent ways. |
|
here #93625, will submit more of those. |
no, those severities are not relevant in this context, they are just the misra severties, probably we should drop this column completely. see this section |
Agreed. If we have our own coding guideline rules that are modeled after MISRA C and thereby all "required," there is no reason to list the MISRA C severity. |
@fabiobaltieri you are "requesting changes" in a PR because of code in another another PR? Please be a bit more specific about your change request. I do not think they are related at all. This is a good catch by @ubieda and probably confusing few other contributors. |
|
I see people focusing in the particular snippet that originated this PR, and yes: of course that can be changed and could be writen better and more concise (which I will do for sure). Please don't assume that I'm doing this because I'm too lazy to fix my code: that is absolutely not true. The reason I filed this PR is because, today, the combination of Checkpatch's
and that's just confusing for developers trying to follow all the guidelines. Moreover, there are instances in the code-base that, if commited today, would break checkpatch compliance, while meeting the code-guideline (again, irrespective if you consider them as code-style worthy or not): zephyr/subsys/net/l2/ieee802154/ieee802154_shell.c Lines 146 to 160 in 4867d45 zephyr/subsys/bluetooth/host/smp.c Lines 3219 to 3229 in 4867d45 zephyr/subsys/bluetooth/host/shell/bt.c Lines 1754 to 1764 in 4867d45 zephyr/subsys/testsuite/ztest/src/ztest_mock.c Lines 94 to 109 in 4867d45 I'm not questioning the rules, neither I am saying that checkpatch isn't useful all in all: I'm questioning how confusing is for developers to attempt to comply with both rules today. The end-result (as I'm sure you can find a lot more instances than the ones I listed above) is that developers tend up to leave much more else-ifs() without If the intend is to discourage people from using else-if's altogether (with a few exceptions), then I'd suggest to just phrase it like that and avoid all this confusion (of course following existing process in place to make it happen). All I want is a set of rules that are clear, and that I know I can follow and be sure I'm writing good, safe and compliant code as per our established guidelines, period. |
@kartben I do not see any derogatory comments from my side. @kartben stop claiming this here in the other context. @kartben stop abusing your position to intimidate me. @kartben stop abusing labels to merge PRs faster according to your preferences. @kartben stop abusing your merge rights to merge PRs with commits that you force-pushed even though they were not approved by the maintainer. @kartben Stop approving PRs whose impact you do not understand. |
That's not the case. Checkpatch is right with
Hey, we also have code that is compliant with "WARNING:UNNECESSARY_ELSE: else is not generally useful after a break or return" and has no issue with MISRA 15.7. It just depends on the author and maintainer. And your examples are not exactly the same as in your PR, there is no if (ret == -EALREADY) {
shell_fprintf(sh, SHELL_INFO,
"Interface is not associated\n");
return -ENOEXEC;
}
if (ret) {
shell_fprintf(sh, SHELL_WARNING,
"Could not disassociate? (status: %i)\n", ret);
return -ENOEXEC;
}
shell_fprintf(sh, SHELL_NORMAL, "Interface is now disassociated\n"); zephyr/subsys/bluetooth/host/smp.c, please take another look. this example has nothing to do with the topic here. if (!strcmp(action, "on")) {
return cmd_active_scan_on(sh, options, timeout);
}
if (!strcmp(action, "off")) {
return cmd_scan_off(sh);
}
if (!strcmp(action, "passive")) {
return cmd_passive_scan_on(sh, options, timeout);
}
shell_help(sh);
return SHELL_CMD_HELP_PRINTED; for (cnt = 0; cnt < words; cnt++) {
neg_bitmap = ~bitmap[cnt];
if (neg_bitmap == 0) {
/* All full. Try next word. */
continue;
}
if (neg_bitmap == ~0UL) {
/* First bit is free */
return cnt * BITS_PER_LONG;
} else {
const unsigned int bit =
(cnt * BITS_PER_LONG) + __builtin_ffsl(neg_bitmap) - 1;
/* Ensure first free bit is within total bits count */
if (bit < bits) {
return bit;
} else {
return -1;
}
}
}Much simpler, and I am pretty sure it ends up with identical opcode. And that illustrates once again the problem with MISRA rules. They are not documented, the documentation cannot be made public, there is no justification for their application, and some rules are not applicable to the project at all. Contributors try to follow the rules without knowing their background. A lack of C knowledge significantly worsens the problem. This cannot be good for the codebase. This cannot be good for safety. Even if someone finally passes compliance runs, it does not make the project safer. It is no justification that these rules were approved at some point. And I am very familiar with the TSC, half of the members have no idea about working on the codebase. Look, MISRA will make code safer, who would be against that? |
|
Unfortunately MISRA-C is sort of the defacto ruleset for a safe C subset, whatever that means. More importantly Zephyr is striving to be safety certified, as this opens yet more areas where Zephyr could be used. Do either @fabiobaltieri or @jfischer-no have a suggestion for a better rule set that would allow for the project to work towards safety certification? I'd love an open rule set that has easy to use open/free tooling. The reality seems to be (I'm no expert) that safety has all sorts of middlemen striving to collect big paychecks. The rules (MISRA), standards (IEC/ISO), tools (cppcheck/astree/sonar/etc) and toolchains (iar/green hills/etc) all have costs, some unlisted (likely shockingly expensive...) that implement all this stuff. I don't think fighting MISRA-C or this ecosystem is a winning argument here @jfischer-no despite the entire enterprise perhaps being questionable and the rulesets at times being questionable. Zephyr would be fighting the entire safety world to do so. That's not a tenable approach in my humble opinion. So why not follow the MISRA rules and tools here? |
|
I don't seem to be able to find the code in question, so it is a little hard to understand if this is the right way to fix this. The question is, would we rather people make their code clearer by restructuring the logic, or does adding the empty else make it clearer. |
|
saying that again, regardless of the change or PR that triggered this, we have a conflict in rules, if rules were enforced, this would be blocking progress. Rules will be enforced, so we should evaluate and fix issues. The checkpatch rules/check is driven by style, i.e.: so this is about avoiding extra indentation and to improve clarity by reducing nesting. There is NOTHING wrong with an else after a return. The zephyr coding guideline (derived from misra and also available in other rule sets) is about logical completeness here we are explicitly asked to check for the unexpected condition and handle it appropriately, so, the rule is expecting this: This is not about style, it is about logical completness. Also note that MISRA does not really mind the else after return because if you follow other rules, i.e. single point of return (rule 15.6) you will never end up having this code in the first place. Checkpatch was added very early own as a one stop shop for all our style requirement, it was the only compliance check for a long period of time. We have evolved since then, we have disabled my checkpatch rules and added our own and we have our own guidelines meanwhile, selected and reviewed by the project and now we find out on the verge of starting to enforce those rules that we have conflicts and confusion in PRs. We should remove this conflict unless someone is willing to fight for removing this guideline (the one in our coding guidelines), but until then, UNNECESSARY_ELSE should be disabled in checkpatch. @keith-zephyr Can you please take necessary action on this PR so we can move forward. I personally think the requests for changes should be dismissed and this PR should be merged. If someone has issues with lack of documentation, documentation for the source of the rules being not public and other girevances, this is probably something to be dealt with somewhere else, not here. |
|
On the topic of documentation, in case anyone on this thread is interested: #93639 |
@nashif - I think you meant rule 15.5, which isn't part of Zephyr's guidelines. Guard clauses use multiple returns to improve readabilty, and in my opinion are a good reason why Zephyr should not adopt MISRA 15.5 Ultimately, the I'm open to formalizing I'm in favor of removing @jfischer-no or @fabiobaltieri - what are your thoughts? |
|
I find this rules helps following the "normal execution inline" concept, so if you see it you should probably not be using an "else if". Fine to remove it, more work on code reviewers.
Yes this check detect the "you should have not ended up having this code". |
yeah, thats why i was talking about MISRA in general and not the Zephyr coding guidelines. |
@keith-zephyr I have not read anything that supports this. The check does not prevent one from writing MISRA-compliant code, and the check is more complementary to rule 15.7, as it detects unnecessary “else blocks” and thus forces code to be written more simply and prohibits cheating (empty else {} blocks). Writing simple code is essential for MISRA and safety compliance. But regardless of whether there is a conflict, this check has been part of the project for a very long time, long before MISRA coding guidelines rules were introduced. The MISRA rules must be tailored to the existing checks and rules. The 15.7 rule is not mandatory, which is decisive here and in all further discussions about the MISRA rules. Also, out of consideration for MISRA coding guidelines, the existing rules and checks that have been in place since the introduction of MISRA rulse cannot be changed at will, as is being attempted here. Changes that have already been made should be investigated and possibly reversed. I quote from the highly readable paper "Bringing Existing Code into MISRA Compliance: Challenges and Solutions", https://www.bugseng.com/bringing-existing-code-into-misra-compliance-challenges-and-solutions/ "When a project has its own established measures to prevent |
ok, i can work with that and discuss the viability of having both rules, this is the most productive comment in the context sofar. Some of the tooling we have can deal with exceptions and deviations.
The documented zephyr coding guidelines overrule anything else, the checkpatch stuff are rules that are part of a tool we used as a whole, many of the rules in checkpatchj do not apply to Zephyr, are Linux specific etc. etc. A few conflict with our coding guidelines and we need to evaluate them and set things straight, we can't have conflicting rules and checkpatch should not be seen or taken as the gold standard that everything needs to follow. I will open a different issue to look at checkpatch and eva;uate rules and how they apply to Zephyr. We do need some cleanup and we need to avoid any confusing.
All listed rules in the Zephyr coding guidelines are mandatory in the context of Zephyr. The MISRA severity was added to the table for reference only. We picked both mandatory, advisory and other rules from MISRA, so in the context of Zephyr, everything is mandatory and will be enforced. We are not trying to be MISRA compliant.
What is being attempted here? What changes are you referring to?
What changes?
I could not agree more. https://github.com/zephyrproject-rtos/zephyr/blob/main/cmake/sca/eclair/ECL/deviations.ecl |
In order to allow the code to comply with MISRA 15.7, which states:
"All else if constructs shall be terminated with an else statement"
This is listed in our Coding Guidelines: https://docs.zephyrproject.org/latest/contribute/coding_guidelines/index.html
Before this patch, the following message will pop-up and fail compliance checks:
Reference: https://github.com/zephyrproject-rtos/zephyr/actions/runs/16481684493/job/46597371633?pr=93166