[haiku-commits] Re: haiku: hrev49508 - src/kits/media
- From: Stephan Aßmus <superstippi@xxxxxx>
- To: haiku-commits@xxxxxxxxxxxxx
- Date: Wed, 5 Aug 2015 16:39:38 +0200
Hi Dario,
Am 05.08.2015 um 13:50 schrieb Dario Casalinuovo:
The point of code readability is to make it obvious. And to be
honest, renaming lateness to "tempLateness", as you did, doesn't
make it any better. You're still assigning "waitUntil" to it, a
value which is not lateness, so the variable name makes no sense.
This variable called before lateness has the unique scope to go in a
function called DispatchMessage that accepts a parameter called
lateness. I can't think of another name. I'm surprised, i posed serious
questions such as calling TimeSource() more than one time, and we are
already in this non-sense?
You still misunderstand, and I don't mean that in an offensive way.
Please don't be defensive, the problem is simply that you don't yet
understand what we are talking about with this specific point. I will
point it out clearly in the current version of the code:
> err = WaitForMessage(waitUntil);
> if (err == B_TIMED_OUT) {
> media_timed_event event;
> if (hasEvent)
> err = fEventQueue.RemoveFirstEvent(&event);
> else
> err = fRealTimeQueue.RemoveFirstEvent(&event);
>
> if (err == B_OK) {
> tempLateness -= TimeSource()->RealTime();
> if (tempLateness < 0)
> tempLateness = 0;
>
> DispatchEvent(&event, tempLateness, hasRealtime);
A clear version of the code would be this:
bigtime_t lateness = waitUntil - TimeSource()->RealTime();
if (lateness < 0)
lateness = 0;
DispatchEvent(&event, lateness, hasRealtime);
You don't need "tempLateness" at all, since "waitUntil" cannot be
"B_INFINITE_TIMEOUT" when in this block of code. I know that "waitUntil"
can be 0, please read further.
> }
> } else if (err != B_OK)
> return;
>
> [...]
>
> if (hasRealtime) {
> waitUntil = fRealTimeQueue.FirstEventTime()
> - fSchedulingLatency;
> }
>
> tempLateness = waitUntil;
Here "tempLateness" does not contain a lateness (delta), but is actually
the original "waitUntil". I hope you agree that this is confusing.
> if (waitUntil < TimeSource()->RealTime())
> waitUntil = 0;
You should not need to overwrite "waitUntil" with 0 when the real time
is already later than "waitUntil". Either WaitForMessage() should be
fixed to do it itself, or whatever kernel API it is based on probably
already deals correctly with timeouts that are in the past. If you
really do need to pass 0 to WaitForMessage(), then you could rename
"tempLateness" to "eventRealTime", which would actually improve the code.
BTW, I think the following would be a much more clear version of
checking hasEvent and hasRealtime:
if (!hasEvent && !hasRealtime) {
// No events, go back to sleep until events are queued
waitUntil = B_INFINITE_TIMEOUT;
continue;
}
if (hasEvent) {
// The time for the next non-realtime event has to be computed
// in any case, since it is needed to see whether a real-time
// event should precede it.
waitUntil = TimeSource()->RealTimeFor(
fEventQueue.FirstEvent()->event_time,
fEventLatency + fSchedulingLatency);
}
if (hasRealtime) {
// See if the realtime event should run first.
// Maintain hasEvent and hasRealtime depending on which
// event shall run.
bigtime_t nextRealtimeEvent = fRealTimeQueue.FirstEventTime()
- fSchedulingLatency;
if (hasEvent) {
if (nextRealtimeEvent <= waitUntil) {
waitUntil = nextRealtimeEvent;
hasEvent = false;
} else
hasRealtime = false;
} else
waitUntil = nextRealtimeEvent;
}
It has less duplicate code, including less checks and one less
computation of the next realtime event time.
Please allow me a remark on your communication with Julia. I think you
are way too defensive and emotional. You are not being attacked. Just
keep the discussion technical and I am sure we can find the best solution.
Best regards,
-Stephan
Other related posts:
