Hi, Here is updated version of change. Ok to commit? thanks, Dinar. On Mon, Sep 2, 2013 at 7:08 PM, Joseph S. Myers wrote: > On Mon, 2 Sep 2013, Dinar Temirbulatov wrote: > >> Hi, >> Another version of change, I added >> atomic_compare_and_exchange_val_acq/atomic_compare_and_exchange_val_rel >> and atomic_compare_and_exchange_bool_acq/atomic_compare_and_exchange_boot_rel >> defenitions and also for gcc-4.7 and higher in the case of unsupported >> atomic compare and swap operation, it uses the kernel helper inlines. >> Tested on arm a9 with no new regressions. Ok to commit? >> Oh, sorry. I missed to attach the change. Here it is. > > For subsequent patch revisions, please note there should be an extra space > after "#" for preprocessor directives inside #if conditionals, one per > level of #if nesting (other than toplevel multiple-inclusion guards) - > which means that if conditioning existing code, you need to adjust > directives inside that code. > > This patch appears to have too much duplication. For example, you > duplicate the definition of __arm_assisted_compare_and_exchange_val_32_acq > - but that should not need any extra conditionals at all (beyond the > existing #ifndef __arm_assisted_compare_and_exchange_val_32_acq), there's > no reason ever not to define it. Similarly, you duplicate > __arch_compare_and_exchange_val_64_acq, but with proper #if structure > there should only need to be one copy of the version that uses > __arm_link_error. > > What I think you should aim for is that each definition, or small group of > definitions, uses conditionals in the form > > #if __GNUC_PREREQ (4, 7) && defined __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 > /* Version using __atomic_*. */ > #elif defined __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 > /* Version using __sync_*. */ > #else > /* Version using __arm_assisted_*. */ > #endif > > with cases omitted if not useful for that particular macro (this may > include some macros not being defined at all in some cases). So don't > insert any global conditionals affecting all the existing definitions at > all - look at each block of conditionals and add a third case as needed, > along with any new macros (again with conditionals in that form) that are > appropriate. > > Where you use abort () in some definitions, use __arm_link_error () > instead, like for the existing definitions. > > -- > Joseph S. Myers > joseph@codesourcery.com