[PATCH] GSOC2012 project atomic support for RTEMS

wei.a.yang at gmail.com wei.a.yang at gmail.com
Thu Sep 20 14:32:26 UTC 2012


Hi Joel, thank you very much for review my patch. I will correct the patch according to your comments ASAP. And I have already updated the wiki for this project. See: http://wiki.rtems.org/wiki/index.php/Atomic_Operations . If there is any lost I can add it. Sorry for reply this so later.

在 2012-9-19,19:47,Joel Sherrill <joel.sherrill at oarcorp.com> 写道:

> 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.
> + rtems/score/atomic.h
>    - 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
>     here.
>   - line up parameter names in prototypes.
> + atomic.inl
>   - 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 ...
>         if X
>            return Y
>         else
>            return X
>      is written
>          if X
>             return Y
>          return X
>     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
> + powerpc
>    - 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. :)
> 
> Thanks.
> 
> --joel
> On 09/18/2012 10:50 AM, yangwei weiyang wrote:
>> Hi
>> 
>> 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
> 
> 
> <white.txt>




More information about the devel mailing list