Re: rtems/src/scheduler* code convention issue
On Fri, May 30, 2014 at 9:24 AM, Joel Sherrill
wrote:
>
> On 5/30/2014 8:13 AM, Gedare Bloom wrote:
>> On Fri, May 30, 2014 at 9:04 AM, Joel Sherrill
>> wrote:
>>> On 5/30/2014 2:43 AM, Peter Dufault wrote:
On May 29, 2014, at 17:47 , Gedare Bloom wrote:
> You understood precisely. I think it was not permitted in C90 perhaps?
> We should revisit it, but I think historically all variables are
> declared at the top of the function.
I'll dig up K&R, but I'm 95% sure it's been supported from the beginning.
What hasn't always been supported is declaring variables anywhere other
than after an opening bracket:
/* Local block */
{
invoke_foo();
int i;
invoke_bar(&i);
/* ... */
}
The argument for permitting declaration inside brackets (not as above, but
as allowed by K&R) is to declare variables in as restrictive a scope as
possible. The argument against is shadowing variables and getting subtle
errors.
>>> I don't know if we have it on but gcc has a shadowing warning which
>>> is pretty effective. I did a code review for a project and learned they
>>> had disabled warnings "because there were too many." When I turned
>>> them back on, there were 1000s and among them:
>>>
>>> + parameters shadowing global variables.
>>> + outer scope local variables shadowing global variables.
>>> + inner scope local variables (from macros) shadowing local variables
>>>
>>> GCC spotted all of the above. I am pretty confident in GCC's shadowing
>>> detection. If we want to allow/encourage this, we need to turn
>>> the warning on.
>>>
>> Thanks for the discussion on this. I will put a rule as declaring
>> variables at the start of a block, and not to shadow variables.
>>
>> Whether we want to encourage variables in nested scopes remains open
>> for discussion, but I think we should allow it at least.
>>
>>> Also we need to remember to write down that macro parameters
>>> start with an _ so as to avoid name collision. The project I reviewed
>>> didn't follow that rule. They also didn't have naming rules which
>>> prevented collisions between local, global, and parameters.
>> I'll add a note about it.
> In case there is not a rule about naming. The public API routines follow
> a standard API like POSIX or *BSD or start with rtems_. In the 4.7-4.9
> timeframe, I cleaned up a lot of support code to do this like the
> stack checker, cpu use, period use, etc.
>
> If a method starts with rtems_, then it should be assumed to be
> available for use by the application and have documentation in
> the User's Guide.
>
There is now a page about naming, linked from the coding conventions:
http://www.rtems.org/wiki/index.php/Naming_rules
> Internal methods (inside score, sapi, rtems, and posix with
> hope that libcsupport and RTEMS specific filesystems and other
> cpukit code follow along) are named by "package" and "method".
> If you think C++, you get package::method. But _[A-Z] indicates
> where a namespace separator would be. The leading _ makes it
> private.
>
I don't quite follow, perhaps you should read the naming rules and
make sure they are consistent with your thinking. My understanding is
that everything that is internal starts with an underscore. There is
no distinction about "private" (in the OOP sense), and any internal
RTEMS code can call any other internal RTEMS code. That said, there
are some layering rules about what code can call what code, but I
don't see them codified anywhere. I have a section set-aside for those
rules when someone gets time to define them.
-Gedare
> _Chain_Get_first is a private method named Get_first in Chain.
> In C++, it might be _Chain::Get_first.
>
> Many modules are essentially classes which encapsulate a
> data structure and all accesses with methods. The first
> parameter to most methods is then going to be logically
> an instance pointer like C++ this.
>
> I don't know if I am rambling, repeating things documented
> now, or something else. But hopefully core dumping like
> this helps.
>> -Gedare
>>
Peter
-
Peter Dufault
HD Associates, Inc. Software and System Engineering
>>> --
>>> Joel Sherrill, Ph.D. Director of Research & Development
>>> joel.sherr...@oarcorp.comOn-Line Applications Research
>>> Ask me about RTEMS: a free RTOS Huntsville AL 35805
>>> Support Available(256) 722-9985
>>>
>
> --
> Joel Sherrill, Ph.D. Director of Research & Development
> joel.sherr...@oarcorp.comOn-Line Applications Research
> Ask me about RTEMS: a free RTOS Huntsville AL 35805
> Support Available(256) 722-9985
>
___
rtems-devel mailing list
rtems-devel@rtems.org
http://www.rtems.org/mailman/listinfo/rtems-devel
Re: rtems/src/scheduler* code convention issue
On 5/30/2014 8:13 AM, Gedare Bloom wrote:
> On Fri, May 30, 2014 at 9:04 AM, Joel Sherrill
> wrote:
>> On 5/30/2014 2:43 AM, Peter Dufault wrote:
>>> On May 29, 2014, at 17:47 , Gedare Bloom wrote:
>>>
You understood precisely. I think it was not permitted in C90 perhaps?
We should revisit it, but I think historically all variables are
declared at the top of the function.
>>> I'll dig up K&R, but I'm 95% sure it's been supported from the beginning.
>>> What hasn't always been supported is declaring variables anywhere other
>>> than after an opening bracket:
>>>
>>> /* Local block */
>>> {
>>> invoke_foo();
>>> int i;
>>> invoke_bar(&i);
>>> /* ... */
>>> }
>>>
>>> The argument for permitting declaration inside brackets (not as above, but
>>> as allowed by K&R) is to declare variables in as restrictive a scope as
>>> possible. The argument against is shadowing variables and getting subtle
>>> errors.
>> I don't know if we have it on but gcc has a shadowing warning which
>> is pretty effective. I did a code review for a project and learned they
>> had disabled warnings "because there were too many." When I turned
>> them back on, there were 1000s and among them:
>>
>> + parameters shadowing global variables.
>> + outer scope local variables shadowing global variables.
>> + inner scope local variables (from macros) shadowing local variables
>>
>> GCC spotted all of the above. I am pretty confident in GCC's shadowing
>> detection. If we want to allow/encourage this, we need to turn
>> the warning on.
>>
> Thanks for the discussion on this. I will put a rule as declaring
> variables at the start of a block, and not to shadow variables.
>
> Whether we want to encourage variables in nested scopes remains open
> for discussion, but I think we should allow it at least.
>
>> Also we need to remember to write down that macro parameters
>> start with an _ so as to avoid name collision. The project I reviewed
>> didn't follow that rule. They also didn't have naming rules which
>> prevented collisions between local, global, and parameters.
> I'll add a note about it.
In case there is not a rule about naming. The public API routines follow
a standard API like POSIX or *BSD or start with rtems_. In the 4.7-4.9
timeframe, I cleaned up a lot of support code to do this like the
stack checker, cpu use, period use, etc.
If a method starts with rtems_, then it should be assumed to be
available for use by the application and have documentation in
the User's Guide.
Internal methods (inside score, sapi, rtems, and posix with
hope that libcsupport and RTEMS specific filesystems and other
cpukit code follow along) are named by "package" and "method".
If you think C++, you get package::method. But _[A-Z] indicates
where a namespace separator would be. The leading _ makes it
private.
_Chain_Get_first is a private method named Get_first in Chain.
In C++, it might be _Chain::Get_first.
Many modules are essentially classes which encapsulate a
data structure and all accesses with methods. The first
parameter to most methods is then going to be logically
an instance pointer like C++ this.
I don't know if I am rambling, repeating things documented
now, or something else. But hopefully core dumping like
this helps.
> -Gedare
>
>>> Peter
>>> -
>>> Peter Dufault
>>> HD Associates, Inc. Software and System Engineering
>>>
>> --
>> Joel Sherrill, Ph.D. Director of Research & Development
>> joel.sherr...@oarcorp.comOn-Line Applications Research
>> Ask me about RTEMS: a free RTOS Huntsville AL 35805
>> Support Available(256) 722-9985
>>
--
Joel Sherrill, Ph.D. Director of Research & Development
joel.sherr...@oarcorp.comOn-Line Applications Research
Ask me about RTEMS: a free RTOS Huntsville AL 35805
Support Available(256) 722-9985
___
rtems-devel mailing list
rtems-devel@rtems.org
http://www.rtems.org/mailman/listinfo/rtems-devel
Re: rtems/src/scheduler* code convention issue
On Fri, May 30, 2014 at 9:04 AM, Joel Sherrill
wrote:
>
> On 5/30/2014 2:43 AM, Peter Dufault wrote:
>> On May 29, 2014, at 17:47 , Gedare Bloom wrote:
>>
>>> You understood precisely. I think it was not permitted in C90 perhaps?
>>> We should revisit it, but I think historically all variables are
>>> declared at the top of the function.
>> I'll dig up K&R, but I'm 95% sure it's been supported from the beginning.
>> What hasn't always been supported is declaring variables anywhere other than
>> after an opening bracket:
>>
>> /* Local block */
>> {
>> invoke_foo();
>> int i;
>> invoke_bar(&i);
>> /* ... */
>> }
>>
>> The argument for permitting declaration inside brackets (not as above, but
>> as allowed by K&R) is to declare variables in as restrictive a scope as
>> possible. The argument against is shadowing variables and getting subtle
>> errors.
> I don't know if we have it on but gcc has a shadowing warning which
> is pretty effective. I did a code review for a project and learned they
> had disabled warnings "because there were too many." When I turned
> them back on, there were 1000s and among them:
>
> + parameters shadowing global variables.
> + outer scope local variables shadowing global variables.
> + inner scope local variables (from macros) shadowing local variables
>
> GCC spotted all of the above. I am pretty confident in GCC's shadowing
> detection. If we want to allow/encourage this, we need to turn
> the warning on.
>
Thanks for the discussion on this. I will put a rule as declaring
variables at the start of a block, and not to shadow variables.
Whether we want to encourage variables in nested scopes remains open
for discussion, but I think we should allow it at least.
> Also we need to remember to write down that macro parameters
> start with an _ so as to avoid name collision. The project I reviewed
> didn't follow that rule. They also didn't have naming rules which
> prevented collisions between local, global, and parameters.
I'll add a note about it.
-Gedare
>> Peter
>> -
>> Peter Dufault
>> HD Associates, Inc. Software and System Engineering
>>
>
> --
> Joel Sherrill, Ph.D. Director of Research & Development
> joel.sherr...@oarcorp.comOn-Line Applications Research
> Ask me about RTEMS: a free RTOS Huntsville AL 35805
> Support Available(256) 722-9985
>
___
rtems-devel mailing list
rtems-devel@rtems.org
http://www.rtems.org/mailman/listinfo/rtems-devel
Re: rtems/src/scheduler* code convention issue
On 5/30/2014 2:43 AM, Peter Dufault wrote:
> On May 29, 2014, at 17:47 , Gedare Bloom wrote:
>
>> You understood precisely. I think it was not permitted in C90 perhaps?
>> We should revisit it, but I think historically all variables are
>> declared at the top of the function.
> I'll dig up K&R, but I'm 95% sure it's been supported from the beginning.
> What hasn't always been supported is declaring variables anywhere other than
> after an opening bracket:
>
> /* Local block */
> {
> invoke_foo();
> int i;
> invoke_bar(&i);
> /* ... */
> }
>
> The argument for permitting declaration inside brackets (not as above, but as
> allowed by K&R) is to declare variables in as restrictive a scope as
> possible. The argument against is shadowing variables and getting subtle
> errors.
I don't know if we have it on but gcc has a shadowing warning which
is pretty effective. I did a code review for a project and learned they
had disabled warnings "because there were too many." When I turned
them back on, there were 1000s and among them:
+ parameters shadowing global variables.
+ outer scope local variables shadowing global variables.
+ inner scope local variables (from macros) shadowing local variables
GCC spotted all of the above. I am pretty confident in GCC's shadowing
detection. If we want to allow/encourage this, we need to turn
the warning on.
Also we need to remember to write down that macro parameters
start with an _ so as to avoid name collision. The project I reviewed
didn't follow that rule. They also didn't have naming rules which
prevented collisions between local, global, and parameters.
> Peter
> -
> Peter Dufault
> HD Associates, Inc. Software and System Engineering
>
--
Joel Sherrill, Ph.D. Director of Research & Development
joel.sherr...@oarcorp.comOn-Line Applications Research
Ask me about RTEMS: a free RTOS Huntsville AL 35805
Support Available(256) 722-9985
___
rtems-devel mailing list
rtems-devel@rtems.org
http://www.rtems.org/mailman/listinfo/rtems-devel
Re: rtems/src/scheduler* code convention issue
On May 29, 2014, at 17:47 , Gedare Bloom wrote:
> You understood precisely. I think it was not permitted in C90 perhaps?
> We should revisit it, but I think historically all variables are
> declared at the top of the function.
I'll dig up K&R, but I'm 95% sure it's been supported from the beginning. What
hasn't always been supported is declaring variables anywhere other than after
an opening bracket:
/* Local block */
{
invoke_foo();
int i;
invoke_bar(&i);
/* ... */
}
The argument for permitting declaration inside brackets (not as above, but as
allowed by K&R) is to declare variables in as restrictive a scope as possible.
The argument against is shadowing variables and getting subtle errors.
Peter
-
Peter Dufault
HD Associates, Inc. Software and System Engineering
___
rtems-devel mailing list
rtems-devel@rtems.org
http://www.rtems.org/mailman/listinfo/rtems-devel
Re: rtems/src/scheduler* code convention issue
On 5/29/2014 4:47 PM, Gedare Bloom wrote:
> On Thu, May 29, 2014 at 5:34 PM, Peter Dufault wrote:
>> On May 28, 2014, at 15:19 , Gedare Bloom wrote:
>>
+ It declares new variables in inner scopes which has been avoided
in the past. I think this was not supported in older C standards and thus
there was no choice but to avoid it. I don't remember when it got added
to C. I assume C99 since that is our target language. But we never
discussed it.
>>> I believe I have seen this creeping into RTEMS recently. The coding
>>> conventions say to use ANSI C, which is a vague description that could
>>> mean C99, or C90.
>> By "declaring variables in inner scopes" are you talking about block scope,
>> that is,
>> int foo(void)
>> {
>> /* And then later on... */
>>
>> {
>> int new_scope_var;
>> ...
>> }
>> }
>> where "new_scope_var" is only in existence in that block?
>>
>> I think that's been around close to forever and it shouldn't be discouraged.
>> Historically macros depend on it, and a coding convention that says
>> "declare a variable in as restricted a scope as possible" is a MUCH better
>> convention than avoiding new variables in block scope.
>>
>> Sorry if I misunderstood.
>>
> You understood precisely. I think it was not permitted in C90 perhaps?
> We should revisit it, but I think historically all variables are
> declared at the top of the function.
Whether it was allowed or not, the original code was just not written to
do that.
And whether we want to do it now or not is a separate issue.
I really don't care.
> -Gedare
>
>> Peter
>> -
>> Peter Dufault
>> HD Associates, Inc. Software and System Engineering
>>
--
Joel Sherrill, Ph.D. Director of Research & Development
joel.sherr...@oarcorp.comOn-Line Applications Research
Ask me about RTEMS: a free RTOS Huntsville AL 35805
Support Available(256) 722-9985
___
rtems-devel mailing list
rtems-devel@rtems.org
http://www.rtems.org/mailman/listinfo/rtems-devel
Re: rtems/src/scheduler* code convention issue
On Thu, May 29, 2014 at 5:34 PM, Peter Dufault wrote:
>
> On May 28, 2014, at 15:19 , Gedare Bloom wrote:
>
>>> + It declares new variables in inner scopes which has been avoided
>>> in the past. I think this was not supported in older C standards and thus
>>> there was no choice but to avoid it. I don't remember when it got added
>>> to C. I assume C99 since that is our target language. But we never
>>> discussed it.
>>>
>> I believe I have seen this creeping into RTEMS recently. The coding
>> conventions say to use ANSI C, which is a vague description that could
>> mean C99, or C90.
>
> By "declaring variables in inner scopes" are you talking about block scope,
> that is,
> int foo(void)
> {
> /* And then later on... */
>
> {
> int new_scope_var;
> ...
> }
> }
> where "new_scope_var" is only in existence in that block?
>
> I think that's been around close to forever and it shouldn't be discouraged.
> Historically macros depend on it, and a coding convention that says "declare
> a variable in as restricted a scope as possible" is a MUCH better convention
> than avoiding new variables in block scope.
>
> Sorry if I misunderstood.
>
You understood precisely. I think it was not permitted in C90 perhaps?
We should revisit it, but I think historically all variables are
declared at the top of the function.
-Gedare
> Peter
> -
> Peter Dufault
> HD Associates, Inc. Software and System Engineering
>
___
rtems-devel mailing list
rtems-devel@rtems.org
http://www.rtems.org/mailman/listinfo/rtems-devel
Re: rtems/src/scheduler* code convention issue
On May 28, 2014, at 15:19 , Gedare Bloom wrote:
>> + It declares new variables in inner scopes which has been avoided
>> in the past. I think this was not supported in older C standards and thus
>> there was no choice but to avoid it. I don't remember when it got added
>> to C. I assume C99 since that is our target language. But we never
>> discussed it.
>>
> I believe I have seen this creeping into RTEMS recently. The coding
> conventions say to use ANSI C, which is a vague description that could
> mean C99, or C90.
By "declaring variables in inner scopes" are you talking about block scope,
that is,
int foo(void)
{
/* And then later on... */
{
int new_scope_var;
...
}
}
where "new_scope_var" is only in existence in that block?
I think that's been around close to forever and it shouldn't be discouraged.
Historically macros depend on it, and a coding convention that says "declare a
variable in as restricted a scope as possible" is a MUCH better convention than
avoiding new variables in block scope.
Sorry if I misunderstood.
Peter
-
Peter Dufault
HD Associates, Inc. Software and System Engineering
___
rtems-devel mailing list
rtems-devel@rtems.org
http://www.rtems.org/mailman/listinfo/rtems-devel
Re: rtems/src/scheduler* code convention issue
I have updated the wiki page with the recommendations Joel listed,
since they seem historical and still are relevant.
http://www.rtems.org/wiki/index.php/Coding_Conventions#Readability
-Gedare
On Wed, May 28, 2014 at 5:15 PM, Joel Sherrill
wrote:
>
> On 5/28/2014 2:21 PM, Gedare Bloom wrote:
>> On Wed, May 28, 2014 at 12:30 PM, Sebastian Huber
>> wrote:
>>> On 2014-05-28 17:43, Gedare Bloom wrote:
> We still have no new rule for "avoidance of deep nesting by using early
>> returns" in the wiki page. So we are still in the situation that
>> everyone
>> interested in code contributions must deduce this rule himself from the
>> "almost 200 API calls which take object ID and have the _Object_Get()
>> switch".
>>
My vote is to limit nesting to 4 levels of indentation. Early returns
or local goto to error/out label should be used when it makes sense.
For example:
int foo(int bar)
{
int a = 0; // one level
if ( bar == a ) {
int b = 1; // two levels
while ( a++ < b ) {
int c = 2; // three levels
if ( a == c ) {
goto err; // fourth and deepest level!
}
}
}
return 0;
err:
return -1;
}
>>> For the cpukit the coding style should enforce that new/changed code looks
>>> similar enough to the exiting code. I really don't care how particular
>>> rules look like since you can find for every rule an arbitrary amount of
>>> pros and cons. I think however that it is very important that the rules in
>>> charge are documented. I should be possible to give newcomers a document
>>> that describes the coding style in detail. It can also be used to judge
>>> code contributions.
>>>
>>> A "avoidance of deep nesting by using early returns" rule should cover what
>>> happens if it leads to duplicated clean up code.
>>>
>> I agree completely. My main point is that I don't know what "deep"
>> means here. You can always refactor code to reduce the depth to 1
>> nesting level, but no one wants that either. I guess what is the
>> prevailing sense of the nesting level we have/want?
> Arbitrary depth isn't the point. It is why you got there. If the logic
> REALLY requires for levels of depth, great. But if the first two are because
> you didn't use an easy case for an early return, then something went wrong.
>
> I am sure that no matter whatever anyone states as a rule, we can find
> some code that violates it. :)
>
> But .. the guidance in the dark ages had some of this:
>
> + parameter checking should be done first and early outs.
>
> + No allocation or critical section entries until all error checking
>that could be done otherwise had been finished.
>
> + Test and action should stay close together. Visually you should
> see action and reaction. Makes code easier to understand.
>
> These generally meant that there you were still at one level
> of indentation when "real work started". And parameters could
> be trusted. This is usually where the "id -> ptr switch" starts.
>
> + Situations vary but, in general, enter critical sections and then
> do error checking which requires locks. Release locks and bail
> out early if they fail.
>
> + I think we generally do no more than two allocation operations
>in the "OS proper". One to allocate an object instance and another
>to allocate some additional memory like for for tasks and
>message queues. This simplifies clean up.
>
> Use of a goto required arguments and justifications. They were very
> much frowned upon.
>
> We also tried to write code which was simple and didn't expect
> much in the way of optimization. I don't know directly how to
> show an example of this.
>
> Another odd one that is value based is to name variables
> and write conditions so the expressions in if's make sense
> when read. If the name and value make it hard to understand,
> you likely made a mistake. Sometimes it is poor name choice
> but in others, it is just bad logic representation. Not a great
> example, but "if car not in park and not in neutral and
> not in reverse" is not as obvious as "if car is in drive"
>
> We write code once but read it many times. There is no such
> thing as code which is too obvious for comments. Readability
> and understandability are critical.
>
> The newer suggestions (hard to say rules on these) which may not
> be followed as well are
>
> + to avoid complex logical expressions in ifs, fors, and whiles.
> These tend to be hard to get coverage for. It is often hard to
> even trip the entire expression.
>
> + avoid inlining methods with complicated logic. This results
> in branch explosion when testing.
>
> + try to avoid duplicating code and use a method.
>
> + Debug assertions are nice.
>
> I think these -- even the suggestions -- are generally followed in
> score, sapi, rtems, posix and probably most of libcsupport.
> Other directories would have to be examined on a c
Re: rtems/src/scheduler* code convention issue
On 5/28/2014 2:21 PM, Gedare Bloom wrote:
> On Wed, May 28, 2014 at 12:30 PM, Sebastian Huber
> wrote:
>> On 2014-05-28 17:43, Gedare Bloom wrote:
We still have no new rule for "avoidance of deep nesting by using early
> returns" in the wiki page. So we are still in the situation that
> everyone
> interested in code contributions must deduce this rule himself from the
> "almost 200 API calls which take object ID and have the _Object_Get()
> switch".
>
>>> My vote is to limit nesting to 4 levels of indentation. Early returns
>>> or local goto to error/out label should be used when it makes sense.
>>> For example:
>>> int foo(int bar)
>>> {
>>>int a = 0; // one level
>>>if ( bar == a ) {
>>> int b = 1; // two levels
>>> while ( a++ < b ) {
>>>int c = 2; // three levels
>>>if ( a == c ) {
>>> goto err; // fourth and deepest level!
>>>}
>>> }
>>>}
>>>return 0;
>>> err:
>>>return -1;
>>> }
>>>
>> For the cpukit the coding style should enforce that new/changed code looks
>> similar enough to the exiting code. I really don't care how particular
>> rules look like since you can find for every rule an arbitrary amount of
>> pros and cons. I think however that it is very important that the rules in
>> charge are documented. I should be possible to give newcomers a document
>> that describes the coding style in detail. It can also be used to judge
>> code contributions.
>>
>> A "avoidance of deep nesting by using early returns" rule should cover what
>> happens if it leads to duplicated clean up code.
>>
> I agree completely. My main point is that I don't know what "deep"
> means here. You can always refactor code to reduce the depth to 1
> nesting level, but no one wants that either. I guess what is the
> prevailing sense of the nesting level we have/want?
Arbitrary depth isn't the point. It is why you got there. If the logic
REALLY requires for levels of depth, great. But if the first two are because
you didn't use an easy case for an early return, then something went wrong.
I am sure that no matter whatever anyone states as a rule, we can find
some code that violates it. :)
But .. the guidance in the dark ages had some of this:
+ parameter checking should be done first and early outs.
+ No allocation or critical section entries until all error checking
that could be done otherwise had been finished.
+ Test and action should stay close together. Visually you should
see action and reaction. Makes code easier to understand.
These generally meant that there you were still at one level
of indentation when "real work started". And parameters could
be trusted. This is usually where the "id -> ptr switch" starts.
+ Situations vary but, in general, enter critical sections and then
do error checking which requires locks. Release locks and bail
out early if they fail.
+ I think we generally do no more than two allocation operations
in the "OS proper". One to allocate an object instance and another
to allocate some additional memory like for for tasks and
message queues. This simplifies clean up.
Use of a goto required arguments and justifications. They were very
much frowned upon.
We also tried to write code which was simple and didn't expect
much in the way of optimization. I don't know directly how to
show an example of this.
Another odd one that is value based is to name variables
and write conditions so the expressions in if's make sense
when read. If the name and value make it hard to understand,
you likely made a mistake. Sometimes it is poor name choice
but in others, it is just bad logic representation. Not a great
example, but "if car not in park and not in neutral and
not in reverse" is not as obvious as "if car is in drive"
We write code once but read it many times. There is no such
thing as code which is too obvious for comments. Readability
and understandability are critical.
The newer suggestions (hard to say rules on these) which may not
be followed as well are
+ to avoid complex logical expressions in ifs, fors, and whiles.
These tend to be hard to get coverage for. It is often hard to
even trip the entire expression.
+ avoid inlining methods with complicated logic. This results
in branch explosion when testing.
+ try to avoid duplicating code and use a method.
+ Debug assertions are nice.
I think these -- even the suggestions -- are generally followed in
score, sapi, rtems, posix and probably most of libcsupport.
Other directories would have to be examined on a case by case basis.
And... Third party code is not modified.
== Random rant on code width
We originally aimed for 1 printed page per function. This is where the
79 columns came from. The length is rather arbitrary. In general
it was from the first line of "real code". When we got off VT100's
and moved to Xterms, it roughly matched the length of an Xterm
from start of method. Which generally is about 50-ish
Re: rtems/src/scheduler* code convention issue
On Wed, May 28, 2014 at 12:30 PM, Sebastian Huber
wrote:
> On 2014-05-28 17:43, Gedare Bloom wrote:
>>>
>>> We still have no new rule for "avoidance of deep nesting by using early
>>> >returns" in the wiki page. So we are still in the situation that
>>> > everyone
>>> >interested in code contributions must deduce this rule himself from the
>>> >"almost 200 API calls which take object ID and have the _Object_Get()
>>> >switch".
>>> >
>>
>> My vote is to limit nesting to 4 levels of indentation. Early returns
>> or local goto to error/out label should be used when it makes sense.
>> For example:
>> int foo(int bar)
>> {
>>int a = 0; // one level
>>if ( bar == a ) {
>> int b = 1; // two levels
>> while ( a++ < b ) {
>>int c = 2; // three levels
>>if ( a == c ) {
>> goto err; // fourth and deepest level!
>>}
>> }
>>}
>>return 0;
>> err:
>>return -1;
>> }
>>
>
> For the cpukit the coding style should enforce that new/changed code looks
> similar enough to the exiting code. I really don't care how particular
> rules look like since you can find for every rule an arbitrary amount of
> pros and cons. I think however that it is very important that the rules in
> charge are documented. I should be possible to give newcomers a document
> that describes the coding style in detail. It can also be used to judge
> code contributions.
>
> A "avoidance of deep nesting by using early returns" rule should cover what
> happens if it leads to duplicated clean up code.
>
I agree completely. My main point is that I don't know what "deep"
means here. You can always refactor code to reduce the depth to 1
nesting level, but no one wants that either. I guess what is the
prevailing sense of the nesting level we have/want?
-Gedare
>
> --
> Sebastian Huber, embedded brains GmbH
>
> Address : Dornierstr. 4, D-82178 Puchheim, Germany
> Phone : +49 89 189 47 41-16
> Fax : +49 89 189 47 41-09
> E-Mail : sebastian.hu...@embedded-brains.de
> PGP : Public key available on request.
>
> Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.
___
rtems-devel mailing list
rtems-devel@rtems.org
http://www.rtems.org/mailman/listinfo/rtems-devel
Re: rtems/src/scheduler* code convention issue
On Wed, May 28, 2014 at 12:12 PM, Joel Sherrill
wrote:
> On 5/28/2014 10:44 AM, Gedare Bloom wrote:
>>
>> On Wed, May 28, 2014 at 11:43 AM, Gedare Bloom wrote:
>>>
>>> On Wed, May 28, 2014 at 11:32 AM, Sebastian Huber
>>> wrote:
On 2014-05-20 20:43, Sebastian Huber wrote:
>
> On 05/20/2014 05:30 PM, Joel Sherrill wrote:
>>
>> On 5/20/2014 10:17 AM, Sebastian Huber wrote:
On 2014-05-20 16:58, Joel Sherrill wrote:
>> 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.
I made several attempts to improve the coding style on the wiki.
This
is an
ongoing process.
>> (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.
There are so many coding styles in the RTEMS code base that it is
hard
to
>>>
>>> guess
the "existing practice".
>>
>> The BSPs and drivers are not good examples of anything and we have all
>> long stated that. The APIs and score portion of cpukit was reasonably
>> consistent.
>>
>> Your response is simply that "I couldn't find the answer so I did what
>> I
>> wanted"
>
>
> You said I violate a rule and I said that there is no such rule in the
> coding
> style
>
> http://www.rtems.org/wiki/index.php/Coding_Conventions
>
> and that there are good reasons to not follow such a rule. It was
> simply
> not
> clear to me that were was a "avoidance of deep nesting by using early
> returns"
> rule. If I look now at the code, then there are indeed a lot of
> examples
> for
> this rule.
>
> All I ask is that rules that should be enforced should be part of the
> coding
> style. This lack of a proper coding style makes it hard for newcomers
> (e.g.
> GSoC students or new colleagues) to get started with RTEMS development.
> I
> leads also to discussions like this.
We still have no new rule for "avoidance of deep nesting by using early
returns" in the wiki page. So we are still in the situation that
everyone
interested in code contributions must deduce this rule himself from the
"almost 200 API calls which take object ID and have the _Object_Get()
switch".
>>> My vote is to limit nesting to 4 levels of indentation. Early returns
>>> or local goto to error/out label should be used when it makes sense.
>>> For example:
>>> int foo(int bar)
>>> {
>>>int a = 0; // one level
>>>if ( bar == a ) {
>>> int b = 1; // two levels
>>> while ( a++ < b ) {
>>>int c = 2; // three levels
>>>if ( a == c ) {
>>> goto err; // fourth and deepest level!
>>>}
>>> }
>>>}
>>>return 0;
>>> e
Re: rtems/src/scheduler* code convention issue
On 2014-05-28 17:43, Gedare Bloom wrote:
We still have no new rule for "avoidance of deep nesting by using early
>returns" in the wiki page. So we are still in the situation that everyone
>interested in code contributions must deduce this rule himself from the
>"almost 200 API calls which take object ID and have the _Object_Get()
>switch".
>
My vote is to limit nesting to 4 levels of indentation. Early returns
or local goto to error/out label should be used when it makes sense.
For example:
int foo(int bar)
{
int a = 0; // one level
if ( bar == a ) {
int b = 1; // two levels
while ( a++ < b ) {
int c = 2; // three levels
if ( a == c ) {
goto err; // fourth and deepest level!
}
}
}
return 0;
err:
return -1;
}
For the cpukit the coding style should enforce that new/changed code looks
similar enough to the exiting code. I really don't care how particular rules
look like since you can find for every rule an arbitrary amount of pros and
cons. I think however that it is very important that the rules in charge are
documented. I should be possible to give newcomers a document that describes
the coding style in detail. It can also be used to judge code contributions.
A "avoidance of deep nesting by using early returns" rule should cover what
happens if it leads to duplicated clean up code.
--
Sebastian Huber, embedded brains GmbH
Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone : +49 89 189 47 41-16
Fax : +49 89 189 47 41-09
E-Mail : sebastian.hu...@embedded-brains.de
PGP : Public key available on request.
Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.
___
rtems-devel mailing list
rtems-devel@rtems.org
http://www.rtems.org/mailman/listinfo/rtems-devel
Re: rtems/src/scheduler* code convention issue
On 5/28/2014 10:44 AM, Gedare Bloom wrote:
On Wed, May 28, 2014 at 11:43 AM, Gedare Bloom wrote:
On Wed, May 28, 2014 at 11:32 AM, Sebastian Huber
wrote:
On 2014-05-20 20:43, Sebastian Huber wrote:
On 05/20/2014 05:30 PM, Joel Sherrill wrote:
On 5/20/2014 10:17 AM, Sebastian Huber wrote:
On 2014-05-20 16:58, Joel Sherrill wrote:
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.
I made several attempts to improve the coding style on the wiki. This
is an
ongoing process.
(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.
There are so many coding styles in the RTEMS code base that it is hard
to
guess
the "existing practice".
The BSPs and drivers are not good examples of anything and we have all
long stated that. The APIs and score portion of cpukit was reasonably
consistent.
Your response is simply that "I couldn't find the answer so I did what I
wanted"
You said I violate a rule and I said that there is no such rule in the
coding
style
http://www.rtems.org/wiki/index.php/Coding_Conventions
and that there are good reasons to not follow such a rule. It was simply
not
clear to me that were was a "avoidance of deep nesting by using early
returns"
rule. If I look now at the code, then there are indeed a lot of examples
for
this rule.
All I ask is that rules that should be enforced should be part of the
coding
style. This lack of a proper coding style makes it hard for newcomers
(e.g.
GSoC students or new colleagues) to get started with RTEMS development. I
leads also to discussions like this.
We still have no new rule for "avoidance of deep nesting by using early
returns" in the wiki page. So we are still in the situation that everyone
interested in code contributions must deduce this rule himself from the
"almost 200 API calls which take object ID and have the _Object_Get()
switch".
My vote is to limit nesting to 4 levels of indentation. Early returns
or local goto to error/out label should be used when it makes sense.
For example:
int foo(int bar)
{
int a = 0; // one level
if ( bar == a ) {
int b = 1; // two levels
while ( a++ < b ) {
int c = 2; // three levels
if ( a == c ) {
goto err; // fourth and deepest level!
}
}
}
return 0;
err:
return -1;
}
Ahem, the goto should be used primarily when there is cleanup to be
done. In this example it would be better to return the -1 directly.
Still, this is my vote.
Agreed on the use of gotos. Avoid.
But whether N levels of depth is appropriate and acceptable ignores the
question
of whether it is "necessary depth". The above example has four levels.
The first
level is not needed:
int a = 0;
int b;
if ( bar == a )
return 0;
b = 1;
while ( a++ < b ) {
int c = 2; // two levels
if ( a == c ) {
return -1;
}
}
return 0;
I don't know if there is a measure of average nesting depth but
it is certainly higher in the original version.
In the original example, the flow is pretty contorted to me with
the goto resulting in two returns back to back.
Ironically, the example also raises three new style questions:
+ It declares new variables in inner scopes which has been avoided
in the past. I think this was not supported in older C standards and thus
there was no choice but to avoid it. I don't remember when it got added
to C. I assume C99 since that is our target language. But we never
discussed it.
+ And there is the question of whether variables should be initialized
when declared or not. I think historically this has been done only for
simple RHS values which are constant so as not to impose order
implications on the declarati
Re: rtems/src/scheduler* code convention issue
On Wed, May 28, 2014 at 11:43 AM, Gedare Bloom wrote:
> On Wed, May 28, 2014 at 11:32 AM, Sebastian Huber
> wrote:
>> On 2014-05-20 20:43, Sebastian Huber wrote:
>>>
>>> On 05/20/2014 05:30 PM, Joel Sherrill wrote:
On 5/20/2014 10:17 AM, Sebastian Huber wrote:
>
> >On 2014-05-20 16:58, Joel Sherrill wrote:
>>
>> >>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.
>
> >I made several attempts to improve the coding style on the wiki. This
> > is an
> >ongoing process.
> >
>>
>> >>(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.
>
> >There are so many coding styles in the RTEMS code base that it is hard
> > to
> guess
> >the "existing practice".
The BSPs and drivers are not good examples of anything and we have all
long stated that. The APIs and score portion of cpukit was reasonably
consistent.
Your response is simply that "I couldn't find the answer so I did what I
wanted"
>>>
>>>
>>> You said I violate a rule and I said that there is no such rule in the
>>> coding
>>> style
>>>
>>> http://www.rtems.org/wiki/index.php/Coding_Conventions
>>>
>>> and that there are good reasons to not follow such a rule. It was simply
>>> not
>>> clear to me that were was a "avoidance of deep nesting by using early
>>> returns"
>>> rule. If I look now at the code, then there are indeed a lot of examples
>>> for
>>> this rule.
>>>
>>> All I ask is that rules that should be enforced should be part of the
>>> coding
>>> style. This lack of a proper coding style makes it hard for newcomers
>>> (e.g.
>>> GSoC students or new colleagues) to get started with RTEMS development. I
>>> leads also to discussions like this.
>>
>>
>> We still have no new rule for "avoidance of deep nesting by using early
>> returns" in the wiki page. So we are still in the situation that everyone
>> interested in code contributions must deduce this rule himself from the
>> "almost 200 API calls which take object ID and have the _Object_Get()
>> switch".
>>
> My vote is to limit nesting to 4 levels of indentation. Early returns
> or local goto to error/out label should be used when it makes sense.
> For example:
> int foo(int bar)
> {
> int a = 0; // one level
> if ( bar == a ) {
> int b = 1; // two levels
> while ( a++ < b ) {
> int c = 2; // three levels
> if ( a == c ) {
> goto err; // fourth and deepest level!
> }
> }
> }
> return 0;
> err:
> return -1;
> }
>
Ahem, the goto should be used primarily when there is cleanup to be
done. In this example it would be better to return the -1 directly.
Still, this is my vote.
-Gedare
>>
>> --
>> Sebastian Huber, embedded brains GmbH
>>
>> Address : Dornierstr. 4, D-82178 Puchheim, Germany
>> Phone : +49 89 189 47 41-16
>> Fax : +49 89 189 47 41-09
>> E-Mail : sebastian.hu...@embedded-brains.de
>> PGP : Public key available on request.
>>
>> Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.
>>
Re: rtems/src/scheduler* code convention issue
On Wed, May 28, 2014 at 11:32 AM, Sebastian Huber
wrote:
> On 2014-05-20 20:43, Sebastian Huber wrote:
>>
>> On 05/20/2014 05:30 PM, Joel Sherrill wrote:
>>>
>>> On 5/20/2014 10:17 AM, Sebastian Huber wrote:
>On 2014-05-20 16:58, Joel Sherrill wrote:
>
> >>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.
>I made several attempts to improve the coding style on the wiki. This
> is an
>ongoing process.
>
>
> >>(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.
>There are so many coding styles in the RTEMS code base that it is hard
> to
guess
>the "existing practice".
>>>
>>> The BSPs and drivers are not good examples of anything and we have all
>>> long stated that. The APIs and score portion of cpukit was reasonably
>>> consistent.
>>>
>>> Your response is simply that "I couldn't find the answer so I did what I
>>> wanted"
>>
>>
>> You said I violate a rule and I said that there is no such rule in the
>> coding
>> style
>>
>> http://www.rtems.org/wiki/index.php/Coding_Conventions
>>
>> and that there are good reasons to not follow such a rule. It was simply
>> not
>> clear to me that were was a "avoidance of deep nesting by using early
>> returns"
>> rule. If I look now at the code, then there are indeed a lot of examples
>> for
>> this rule.
>>
>> All I ask is that rules that should be enforced should be part of the
>> coding
>> style. This lack of a proper coding style makes it hard for newcomers
>> (e.g.
>> GSoC students or new colleagues) to get started with RTEMS development. I
>> leads also to discussions like this.
>
>
> We still have no new rule for "avoidance of deep nesting by using early
> returns" in the wiki page. So we are still in the situation that everyone
> interested in code contributions must deduce this rule himself from the
> "almost 200 API calls which take object ID and have the _Object_Get()
> switch".
>
My vote is to limit nesting to 4 levels of indentation. Early returns
or local goto to error/out label should be used when it makes sense.
For example:
int foo(int bar)
{
int a = 0; // one level
if ( bar == a ) {
int b = 1; // two levels
while ( a++ < b ) {
int c = 2; // three levels
if ( a == c ) {
goto err; // fourth and deepest level!
}
}
}
return 0;
err:
return -1;
}
>
> --
> Sebastian Huber, embedded brains GmbH
>
> Address : Dornierstr. 4, D-82178 Puchheim, Germany
> Phone : +49 89 189 47 41-16
> Fax : +49 89 189 47 41-09
> E-Mail : sebastian.hu...@embedded-brains.de
> PGP : Public key available on request.
>
> Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.
> ___
> rtems-devel mailing list
> rtems-devel@rtems.org
> http://www.rtems.org/mailman/listinfo/rtems-devel
___
rtems-devel mailing list
rtems-devel@rtems.org
http://www.rtems.org/mailman/listinfo/rtems-devel
Re: rtems/src/scheduler* code convention issue
On 2014-05-20 20:43, Sebastian Huber wrote: On 05/20/2014 05:30 PM, Joel Sherrill wrote: On 5/20/2014 10:17 AM, Sebastian Huber wrote: >On 2014-05-20 16:58, Joel Sherrill wrote: >>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. >I made several attempts to improve the coding style on the wiki. This is an >ongoing process. > >>(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. >There are so many coding styles in the RTEMS code base that it is hard to guess >the "existing practice". The BSPs and drivers are not good examples of anything and we have all long stated that. The APIs and score portion of cpukit was reasonably consistent. Your response is simply that "I couldn't find the answer so I did what I wanted" You said I violate a rule and I said that there is no such rule in the coding style http://www.rtems.org/wiki/index.php/Coding_Conventions and that there are good reasons to not follow such a rule. It was simply not clear to me that were was a "avoidance of deep nesting by using early returns" rule. If I look now at the code, then there are indeed a lot of examples for this rule. All I ask is that rules that should be enforced should be part of the coding style. This lack of a proper coding style makes it hard for newcomers (e.g. GSoC students or new colleagues) to get started with RTEMS development. I leads also to discussions like this. We still have no new rule for "avoidance of deep nesting by using early returns" in the wiki page. So we are still in the situation that everyone interested in code contributions must deduce this rule himself from the "almost 200 API calls which take object ID and have the _Object_Get() switch". -- Sebastian Huber, embedded brains GmbH Address : Dornierstr. 4, D-82178 Puchheim, Germany Phone : +49 89 189 47 41-16 Fax : +49 89 189 47 41-09 E-Mail : sebastian.hu...@embedded-brains.de PGP : Public key available on request. Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG. ___ rtems-devel mailing list rtems-devel@rtems.org http://www.rtems.org/mailman/listinfo/rtems-devel
Re: rtems/src/scheduler* code convention issue
On 05/20/2014 05:30 PM, Joel Sherrill wrote: On 5/20/2014 10:17 AM, Sebastian Huber wrote: >On 2014-05-20 16:58, Joel Sherrill wrote: >>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. >I made several attempts to improve the coding style on the wiki. This is an >ongoing process. > >>(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. >There are so many coding styles in the RTEMS code base that it is hard to guess >the "existing practice". The BSPs and drivers are not good examples of anything and we have all long stated that. The APIs and score portion of cpukit was reasonably consistent. Your response is simply that "I couldn't find the answer so I did what I wanted" You said I violate a rule and I said that there is no such rule in the coding style http://www.rtems.org/wiki/index.php/Coding_Conventions and that there are good reasons to not follow such a rule. It was simply not clear to me that were was a "avoidance of deep nesting by using early returns" rule. If I look now at the code, then there are indeed a lot of examples for this rule. All I ask is that rules that should be enforced should be part of the coding style. This lack of a proper coding style makes it hard for newcomers (e.g. GSoC students or new colleagues) to get started with RTEMS development. I leads also to discussions like this. Still no attempt at a community discussion. This statement is not fair. -- Sebastian Huber, embedded brains GmbH Address : Dornierstr. 4, D-82178 Puchheim, Germany Phone : +49 89 189 47 41-16 Fax : +49 89 189 47 41-09 E-Mail : sebastian.hu...@embedded-brains.de PGP : Public key available on request. Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG. ___ rtems-devel mailing list rtems-devel@rtems.org http://www.rtems.org/mailman/listinfo/rtems-devel
Re: rtems/src/scheduler* code convention issue
On Tue, May 20, 2014 at 11:30 AM, Joel Sherrill wrote: > > On 5/20/2014 10:17 AM, Sebastian Huber wrote: >> On 2014-05-20 16:58, Joel Sherrill wrote: >>> 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. >> I made several attempts to improve the coding style on the wiki. This is an >> ongoing process. >> >>> (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. >> There are so many coding styles in the RTEMS code base that it is hard to >> guess >> the "existing practice". > The BSPs and drivers are not good examples of anything and we have all > long stated that. The APIs and score portion of cpukit was reasonably > consistent. > > Your response is simply that "I couldn't find the answer so I did what I > wanted" > Still no attempt at a community discussion. Style issues like this have been discussed on this mailing list, and the wiki page attempts to codify the rules in practice and as we the community want them to be. The fact is that there is no reference to nesting levels in those rules. I think 3 is a reasonable level of nesting. There also is nothing about having a single point of return. I don't think we want to enforce a single point of return, but I'm not going to complain about such code either, unless it really is horrendously nested. >>> 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. >> No, looking at a random set of files to guess the "existing practice" is not >> an >> option. My point is that if you want me to follow a certain rule, then we >> have >> to add this rule to the coding style. I have no problem with a rule like >> "avoidance of deep nesting by using early returns". I just wanted to >> highlight >> that there are some standards that use different rules. > I won't argue the point that this wasn't documented. When asked, > I and other folks have tried to document existing practice. This is an > ongoing effort and something any code base that moves from research > to production has to face. It is a similar issue to have a requirements > set. Research code simply doesn't have DO-178 Level A documentation. > Yes, this is on-going. So let's focus on determining the rules we want to follow, and adding them to our conventions. > FWIW a style rule for safety critical code I think is worth consideration > in some parts of the code base relates to the complexity of expressions > in if's. Compound expressions are harder to do full path coverage on. > Some of the coding guides have rules on this. But I would never force > this on the community without discussion and an open source way to > enforce it. > We need to have a discussion about what we want our style guide to do for us. Make coverage easier? Improve readability? Reduce source lines of code? These priorities may be conflicting in some cases. > I am also convinced that any attempt to enforc
Re: rtems/src/scheduler* code convention issue
On 5/20/2014 10:17 AM, Sebastian Huber wrote: > On 2014-05-20 16:58, Joel Sherrill wrote: >> 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. > I made several attempts to improve the coding style on the wiki. This is an > ongoing process. > >> (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. > There are so many coding styles in the RTEMS code base that it is hard to > guess > the "existing practice". The BSPs and drivers are not good examples of anything and we have all long stated that. The APIs and score portion of cpukit was reasonably consistent. Your response is simply that "I couldn't find the answer so I did what I wanted" Still no attempt at a community discussion. >> 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. > No, looking at a random set of files to guess the "existing practice" is not > an > option. My point is that if you want me to follow a certain rule, then we > have > to add this rule to the coding style. I have no problem with a rule like > "avoidance of deep nesting by using early returns". I just wanted to > highlight > that there are some standards that use different rules. I won't argue the point that this wasn't documented. When asked, I and other folks have tried to document existing practice. This is an ongoing effort and something any code base that moves from research to production has to face. It is a similar issue to have a requirements set. Research code simply doesn't have DO-178 Level A documentation. FWIW a style rule for safety critical code I think is worth consideration in some parts of the code base relates to the complexity of expressions in if's. Compound expressions are harder to do full path coverage on. Some of the coding guides have rules on this. But I would never force this on the community without discussion and an open source way to enforce it. I am also convinced that any attempt to enforce whatever rules we adopt from MISRA, ESA, JSF, NASA, etc. need to be automatically enforced and a subset of the tree deemed "critical" and be enforced. >> 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? > I would not reformat unless this can be done by a tool. > Then we are stuck with inconsistent code or documenting and following the existing rules. -- Joel Sherrill, Ph.D. Director of Research & Development joel.sherr...@oarcorp.comOn-Line Applications Research Ask me about RTEMS: a free RTOS Huntsville AL 35805 Support Available(256) 722-9985 ___ rtems-devel mailing list rtems-devel@rtems.org http://www.rtems.org/mailman/listinfo/rtems-devel
Re: rtems/src/scheduler* code convention issue
On 2014-05-20 16:58, Joel Sherrill wrote: 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. I made several attempts to improve the coding style on the wiki. This is an ongoing process. (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. There are so many coding styles in the RTEMS code base that it is hard to guess the "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. No, looking at a random set of files to guess the "existing practice" is not an option. My point is that if you want me to follow a certain rule, then we have to add this rule to the coding style. I have no problem with a rule like "avoidance of deep nesting by using early returns". I just wanted to highlight that there are some standards that use different rules. 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? I would not reformat unless this can be done by a tool. -- Sebastian Huber, embedded brains GmbH Address : Dornierstr. 4, D-82178 Puchheim, Germany Phone : +49 89 189 47 41-16 Fax : +49 89 189 47 41-09 E-Mail : sebastian.hu...@embedded-brains.de PGP : Public key available on request. Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG. ___ rtems-devel mailing list rtems-devel@rtems.org http://www.rtems.org/mailman/listinfo/rtems-devel
Re: rtems/src/scheduler* code convention issue
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.sherr...@oarcorp.comOn-Line Applications Research
Ask me about RTEMS: a free RTOS Huntsville AL 35805
Support Available(256) 722-9985
___
rtems-devel mailing list
rtems-devel@rtems.org
http://www.rtems.org/mailman/listinfo/rtems-devel
Re: rtems/src/scheduler* code convention issue
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. -- Sebastian Huber, embedded brains GmbH Address : Dornierstr. 4, D-82178 Puchheim, Germany Phone : +49 89 189 47 41-16 Fax : +49 89 189 47 41-09 E-Mail : sebastian.hu...@embedded-brains.de PGP : Public key available on request. Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG. ___ rtems-devel mailing list rtems-devel@rtems.org http://www.rtems.org/mailman/listinfo/rtems-devel
Re: rtems/src/scheduler* code convention issue
Is there a written rule for what constitutes "deep" nesting? I find 3 levels not that deep, albeit if unnecessary. Perhaps the author preferred to have a single exit point? -Gedare On Mon, May 12, 2014 at 7:28 PM, 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. The nesting on schedulergetprocessorset.c is pretty > ugly and could have been avoided easily. > > -- > Joel Sherrill, Ph.D. Director of Research & Development > joel.sherr...@oarcorp.comOn-Line Applications Research > Ask me about RTEMS: a free RTOS Huntsville AL 35805 > Support Available(256) 722-9985 > > ___ > rtems-devel mailing list > rtems-devel@rtems.org > http://www.rtems.org/mailman/listinfo/rtems-devel ___ rtems-devel mailing list rtems-devel@rtems.org http://www.rtems.org/mailman/listinfo/rtems-devel
