rtems/src/scheduler* code convention issue

[Joel Sherrill]

On 5/13/2014 1:16 AM, Sebastian Huber wrote:
> On 2014-05-13 01:28, Joel Sherrill wrote:
>> Hi
>>
>> Both schedulerident.c and schedulergetprocessorset.c do not follow
>> RTEMS Coding Conventions on avoidance of deep nesting by using
>> early returns.
> I don't find such a rule here:
>
> http://www.rtems.org/wiki/index.php/Coding_Conventions
>
>> The nesting on schedulergetprocessorset.c is pretty
>> ugly and could have been avoided easily.
>>
> I prefer a single exit point.  This is also advised by MISRA Rule 15.5.  It is 
> required by ISO 61508 and ISO 26262.  It is also "will" rule (AV Rule 113) of 
> the "JOINT STRIKE FIGHTER AIR VEHICLE C++ CODING STANDARDS FOR THE SYSTEM 
> DEVELOPMENT AND DEMONSTRATION PROGRAM".
>
> http://www.stroustrup.com/JSF-AV-rules.pdf
>
> So if you ask me, then we should also add this to the our coding conventions.
>

I'll be honest. I don't really care what the JSF rules or MISRA rules
say. At this point in the "discussion", what they say is completely
irrelevant to this situation. Whether I like this rule or not, the fact
remains that

(1) There was no community discussion.
(2) There is no way to enforce it (or any other rule)
(3) It is an arbitrary change and leads to inconsistent
code in the code base.

(1) and (3) really bother me.  (2) we have lived with a long time.
I don't like it but we haven't figured out how to solve that.

The only community discussion was me asking why you didn't
follow existing practice.

With no community discussion, this implies that one person can
make decisions for the community. This was a serious decision and
the thread consisted of 3 emails with the first being me asking
why you didn't follow existing practice. There was no discussion
of changing style, how existing code would be impacted, inconsistency
addressed, etc..

That doesn't sound like a healthy discussion to change 25 years
of coding practice -- written style guide or not, you can look at the
code and see it. There are almost 200 API calls which take object ID
and have the _Object_Get() switch. Those are pretty damned
obvious ignoring any other code.

Inconsistent coding is an indication that we are not a community.
This detracts from the code base.  Were you volunteering to
reformat the code to follow the new rule?

Now for some technical details.

Every rule has a plus and minus.  Just because a rule is in
a set doesn't mean that it is the singular answer to
the meaning of life and perfect for all cases.

This style of coding leads to deep nesting which pushes code
farther and farther to the right margin. You have to match
{} from top to bottom of a routine. Take threadsetscheduler.c
and other API level directives for example. The body of the
switch statement is nested one level for every parameter check.
The increased indentation leads to line breaks. I believe the visual
separation of cases which could have been early outs from
their checks decreases the readability of the code.

The current style was chosen because it makes it easier to
ensure that at a certain line of code in a method a certain
set of logical assertions can be made. Before the switch in
the API methods, all parameters that can be checked without
accessing the object have been validated.  The current
style avoids deep nesting which lets you use longer names
and reduce the possibility of a line break.

With our move to more assertions, the paring of the logic
tree as you linearly go through the method gives very nice
places to insert assertions. JPL has found that the percentage
of assertions was a major factor in predicting the number
of defects.

I will agree that either style should have no impact on the
cyclomatic complexity. Unless the complexity of the nesting
to get a single exit point leads to the introduction of method
state variables which have to be checked. I have seen this
before.

-- 
Joel Sherrill, Ph.D.             Director of Research & Development
joel.sherrill at OARcorp.com        On-Line Applications Research
Ask me about RTEMS: a free RTOS  Huntsville AL 35805
Support Available                (256) 722-9985