cleaner & greener (was Re: A patch for RTEMS4.10.0 PowerPC heap space initialization)

[Kate Feng]

Joel Sherrill wrote:
> This is very close for me. 
Please see the attached patch. In addition to Joel's feedback, I trimmed
some code. For RTEMS4.10.1, it is on the safe side to leave
uintptr_t bsp_sbrk_init(uintptr_t, uintptr_t*);
in bootcard.c  because bsp/bootcard.h is called in many other files.
At least, it is within the #ifdef and #endif.
In the future (e.g. RTEMS4.10.2) , it can be added to bsp.h instead.
I will submit a PR for RTETMS4.10.1 if there is no more outstanding issue.

Thanks,
Kate
> If the bsp.h does what it is supposed to, then bootcard.c
> can conditionally compile the code that calls bsp_sbrk_init().
> This results in bsp_sbrk_init() only being required when the
> BSP really supports it.   In addition, if the BSP doesn't support
> sbrk(), then the current simple code can be used.
>
> Either a BSP supports sbrk() or it doesn't.  We need to avoid
> code bloat in BSPs.  It is a continuous battle.  sbrk() is an
> optional feature and when not supported by a BSP, we should
> use the simplest possible code.
>
> I know that is hard to see when all you work on is what are
> the largest RTEMS targets.  But there are plenty of smaller
> ones and flight validated where extra code is undesirable.
>
> This should be a minor change.  Mostly centered around bootcard.c
>> Thanks,
>> Kate
>>
>> Gedare Bloom wrote:
>>> +static uintptr_t heap_sbrk_spared=0;
>>> Why is this declared as static global? It looks to me like it is only
>>> used in one function, can it go on that function's local variables?
>>>
>>> _bsp_sbrk_init()
>>> Should be renamed as bsp_sbrk_init() to be consistent with other bsp 
>>> functions.
>>>
>>> On Fri, May 13, 2011 at 11:08 AM, Kate Feng<feng1 at bnl.gov>  wrote:
>>>
>>>> Sebastian Huber wrote:
>>>>
>>>>> On 05/13/2011 04:13 PM, Kate Feng wrote:
>>>>>
>>>>>
>>>>>> +++ rtems-4.10.0/c/src/lib/libbsp/shared/bootcard.c     2011-05-13
>>>>>> 09:53:27.000000000 -0400
>>>>>> @@ -53,6 +53,7 @@
>>>>>>   #include<bsp/bootcard.h>
>>>>>>   #include<rtems/bspIo.h>
>>>>>> +#include<rtems/malloc.h>
>>>>>>   /*
>>>>>>   *  At most a single pointer to the cmdline for those target
>>>>>> @@ -65,6 +66,9 @@
>>>>>>   */
>>>>>>   extern bool rtems_unified_work_area;
>>>>>>   +static uintptr_t heap_sbrk_spared=0;
>>>>>> +extern uintptr_t _bsp_sbrk_init(uintptr_t, uintptr_t*);
>>>>>>
>>>>>>
>>>>> The _bsp_sbrk_init() function is only available on PowerPC.
>>>>>
>>>>>
>>>>>
>>>> Thanks for your feedback.  How about the attached ?
>>>> I added _bsp_sbrk_init() for other BSPs at
>>>> c/src/lib/libbsp/shared/sbrk.c.  According to Joel, it would be needed
>>>> in the future.  The name seems to be OK at this level, unless Joel 
>>>> comes
>>>> up with a different name. One could fill in the function when the 
>>>> memory
>>>> protection is implemented.  It seems that other
>>>> non-PPC BSPs all use the sbrk() from c/src/lib/libbsp/shared/sbrk.c.
>>>>
>>>> More comments before PR ?
>>>>
>>>> Thanks,
>>>> Kate
>>>>
>>>>
>>>> _______________________________________________
>>>> rtems-users mailing list
>>>> rtems-users at rtems.org
>>>> http://www.rtems.org/mailman/listinfo/rtems-users
>>>>
>>>>
>>>>
>
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: rtems-4.10.0-heapspace_05132011.diff
Type: text/x-patch
Size: 4590 bytes
Desc: not available
URL: <http://lists.rtems.org/pipermail/users/attachments/20110513/0a6b8ce0/attachment-0001.bin>