general comments on sysiinit patches

[Gedare Bloom]

On Wed, Jan 27, 2016 at 9:29 AM, Sebastian Huber
<sebastian.huber at embedded-brains.de> wrote:
>
>
> On 27/01/16 15:01, Joel Sherrill wrote:
>>
>>
>> On Tue, Jan 26, 2016 at 10:02 AM, Sebastian Huber
>> <sebastian.huber at embedded-brains.de
>> <mailto:sebastian.huber at embedded-brains.de>> wrote:
>>
>>     On 26/01/16 16:51, Joel Sherrill wrote:
>>
>>         Hi
>>
>>         I have questions that probably impact all/most of the patches
>>         so thought I would start another thread. Then it is just
>>         detailed review of the individual patches.
>>
>>         + We have used EXTERN to avoid duplicating extern and
>>         instantiating data. You appear to have completely removed that
>>         pattern with no discussion of changing the coding style.  I
>>         really don't like duplication of the information.
>>
>>
>>     There is no duplicate information. We have exactly one declaration
>>     and one definition. The compiler checks that they harmonize. Its
>>     necessary to move the definitions to the right module, so that the
>>     linker can do its job and add the right initialization items.
>>
>> It was formerly one line that did both in a single  location with
>> documentation. The data was instantiated in a per manager file. The pattern
>> was clear since it was used at least twenty times.
>
>
> A single line with some pre-processor magic. For SAPI and score its a global
> switch.
>
>>     This extern stuff is not mentioned here:
>>
>>     https://devel.rtems.org/wiki/Developer/Coding/Conventions
>>
>>
>> Come on.. citing letter of the "law" in this case is a pretty weak
>> defense. There is a lot of stuff not mentioned there and you have been
>> around long enough to know that that particular document has been grown a
>> bit at a time specifically as we realized there is a pattern not covered.
>> You and I both know that it is likely far from complete. And using "not
>> mentioned" when there is clearly an existing pattern in the code itself is
>> disingenuous.
>>
>> What a compiler may check and duplicating information are different
>> things. You have changed the pattern in at least two ways:
>>
>> + dedicated file for data instantiation per manager/handler
>> + single line to serve both as declaration and instantiation
>>
>> The two patterns have similar technical effect in that they declare and
>> instance a variable. But you changed the pattern with no discussion about
>> the pattern. That is the issue here.
>
>
> I didn't want to change a pattern. My goal was to implement the linker set
> based initialization which I thought was consensus. For this its important
> that a certain global variable definition and its initialization
> item/handler is in the same module. The mechanism is that the user of a
> certain functional unit reference some global data of this functional unit.
> This pulls in the initialization item/handler for this functional unit. For
> the RTEMS managers this is usually quite easy, just place the initialization
> item/handler to the definition of the objects information definition. With
> this pre-processor magic to define global data you make it quite hard to
> review this part. In how many other software projects is this mechanism to
> define global data used? It confuses source code reference tools. If you see
> this stuff the first time, how long do you need to understand what is going
> on here? If you insist, then I can bring back this pattern, but I don't
> think it makes things more clear.
>
I spent > 1 hour staring at EXTERN the first time I encountered it,
and still didn't understand until I asked, and was still a bit
confused on the whole subject. What is the benefit to maintaining this
pattern (Joel)?

I'm good with changing the pattern, but I do agree that we ought to be
clear this is a modification to how global variables have in the past
primarily been declared/defined.

Gedare