Re: [OpenBIOS] [PATCH] Adds the get-time word to the dictionary. Used to obtain the current time.

[Programmingkid]

On Oct 4, 2012, at 3:30 PM, [e-mail] wrote:
> Message: 5
> Date: Tue, 25 Sep 2012 16:03:20 +0200
> From: Segher Boessenkool 
> To: The OpenBIOS Mailinglist 
> Subject: Re: [OpenBIOS] [PATCH] Adds the get-time word to the
>    dictionary.	Used to obtain the current time.
> Message-ID: 
> Content-Type: text/plain; charset=US-ASCII; delsp=yes; format=flowed
> > : get-time
> >   " rtc" open-dev                   ( ihandle )
> >   ?dup 0= if
> >      cr abort" Sorry but no rtc node is present on this system"
> >   then                              ( ihandle )
> > > r " get-time" r@ $call-method    ( <returned args - 6 of em?
> >
> > Yikes > )
> >   r> close-dev                      ( <returned args> )
> > ;
>
> : get-time
>   s" rtc" s" get-time" execute-device-method
>   0= ABORT" Sorry but no rtc node is present on this system" ;
>
> (note that ABORT" takes a flag as input), but better is
>
> : get-time
>   s" rtc" s" get-time" execute-device-method
>   0= IF fake-some-sort-of-plausible-time-1970-or-so THEN ;
>
> which is pretty much what Apple's OF does.  execute-device-method
> is not the same thing as open-dev followed by $call-method: it
> does not call "open" in the instance for the device itself (you
> only need that for devices that can have multiple clients at the
> same time and need to keep state for each client; or when instead
> you need to prevent concurrent access from multiple clients, by
> having the second open fail).
>
> Segher

I honestly see no reason to replace open-dev with execute-device-method. My original patch is just fine.

[PATCH] Adds the get-time word to the dictionary. Used to obtain the current time.

[Tarl Neustaedter]

On 2012-Oct-6 10:09 , Programmingkid wrote:
> I honestly see no reason to replace open-dev with 
> execute-device-method. My original patch is just fine.

In Forth, one of the general rules of programming is "fewer words is 
better". In particular, "string execute-device-method" is better than 
"open-dev dup >r if abort else string r@ call-method then".

The kind of review comments you've been getting here (and mostly 
dismissing) are normal in any shared engineering project. This is part 
of the normal give-and-take to produce a body of code that everyone is 
comfortable maintaining.

When examining someone else's forth code, if I see a long complicated 
sequence where a shorter one would have sufficed, I generally assume 
there was a necessity of using the longer one, and look to see what that 
necessity was. That can cause me to waste time.

As to the overall issue of adding get-time to the top-level dictionary. 
I don't know what the rules are in Openbios, but in Openboot, the 
general assumption is you _don't_ add words to the top level dictionary 
without an ARC (architecture review committee) case, and in the past, 
sending it to the IEEE 1275 committee as well. The preference is that 
anything in the top-level dictionary _should_ be documented in the 1275 
specification or one of its updates.

That was back in the days where we wanted to keep various 
implementations of openfirmware inter-compatible. No longer a big issue, 
but in general it would be worthwhile determining what the policy of 
this particular codebase is before submitting changes.