[PATCH] GSOC2012 project atomic support for RTEMS
joel.sherrill at OARcorp.com
Wed Sep 19 11:47:05 UTC 2012
The first thing I noticed was that git complained about white
space issues. I have attached the output from that.
Here are random comments as I spotted them beyond that.
+ I ran all tests on powerpc/psim and the atomic tests passed.
+ Check the copyright year to 2012 everywhere.
- Capitalize the first letter in sentences in comments.
- }Atomic_Memory_barrier needs a space after }
- Each Atomic Load, Store, etc method needs its own
Doxygen comment block. There is a Doxygen command
to reuse another comment (I think) which might be useful
- line up parameter names in prototypes.
- Copyright and file description go in separate comment blocks
- Doxygen file header goes first
- Stay < 80 columns
- acq_XXX? Can we spell that out completely, please? In RTEMS
we do not abbreviate.
- Ditto for "rel"
- Ditto for "ptr"
- Ditto on others I might have missed
- if( should be if (
- normally ...
Notice that the else is not needed and we avoid indenting more.
This will really clean up a method like _Atomic_Fetch_add_int
- Again no Doxygen comments on methods
- Single line only -- no need for > 1 blank line
- line up arguments
+ i386 file
- spacing, abbreviation
- watch \ at end of line to make sure they line up. They don't
- Same as i386
I didn't have as much time to spend on the test code.
- no intermediate blank lines in output.
- copyright year
- no double "*" lines in comments
- What's the purpose of TASK_NUMS? Better name?
- Why is the CONFIGURE_MAXIMUM_TASKS not near the rest of the
parameters? Comment it in the "odd" place and explain near the
rest of the parameters where it is. If it is needed to do this by the
test design (e.g. file reuse, etc). Try to avoid it.
- ATOMIC_STORE_NO_BARRIER > 80 columns
Please also take the time to update the Wiki page for this as a project.
It clearly has made a lot of progress but since it only supports 2 target
architectures, there is work left.
Mostly just clean up. The technical side of this got beaten mercilessly
during the summer. :)
On 09/18/2012 10:50 AM, yangwei weiyang wrote:
> There are some patches to RTEMS which need review.
> It is the final version of GSOC2012 project atomic support for RTEMS.
> This series include six patches:
> -The first patch includes all the modifies to the related *.am and *.ac files.
> -The second patch includes the atomic operations API definition for
> all the architectures.
> -The third patch includes the implementation of atomic operations
> for i386 architecture.
> -The fourth patch includes the implementation of atomic operations
> for PowerPC architecture.
> -The fifth patch includes seven test cases of atomic operations for UP system.
> -The sixth patch includes seven test cases of atomic operations for
> SMP system.
> Any suggestion is welcome!
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
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
More information about the devel