[mythtv] [PATCH] fix auto-transcode for "find once" schedules

[Joseph A. Caputo]

On Tuesday 21 June 2005 10:13, Robert Tsai wrote:
> On Tue, Jun 21, 2005 at 02:15:53AM -0400, Chris Pinkham wrote:
> > Can you take a look at the attached patch and see if it looks like
> > it accomplishes the same thing you were going for.  I have had a
> > couple complaints about the same bug with commercial flagging and
> > hadn't had a chance to investigate yet.
> 
> Your patch works for me (I tested it on a garbage 5-minute "find-once"
> recording this morning).
> 
> > I think this makes the code a little cleaner and fixes the bug you
> > were trying to fix as well as the one I needed to fix. :)
> 
> I agree. I have some small comments:
> 
> > +#define ClearJobMask(mask)               mask = JOB_NONE;
> > +#define JobIsInMask(job, mask)           ((bool)(job & mask))
> > +#define JobIsNotInMask(job, mask)        (!(JobIsInMask(job, 
mask)))
> > +#define AddJobsToMask(jobs, mask)        mask |= jobs;
> > +#define RemoveJobsFromMask(jobs, mask)   mask &= (~jobs);
> > +
> 
> The macro args should (always, IMHO) be parenthesized. Otherwise,
> things like:
> 
> 	JobIsInMask(job, JOB_TRANSCODE | JOB_COMMFLAG)
> 	RemoveJobsFromMask(jobs, JOB_TRANSCODE | JOB_COMMFLAG)
> 
> will not work as expected, because they will have textually expanded
> to become:
> 
> 	((bool)(job & JOB_TRANSCODE | JOB_COMMFLAG))
> 	jobs &= (~JOB_TRANSCODE | JOB_COMMFLAG)
> 
> which is probably not what the programmer wants ('&' and '~' bind more
> tightly than '|').
> 
> Another option would be to just use subroutines (inlined, if the
> performance of these statements really really matters to you).

I'll second that -- I'd go with inline functions, if performance is that 
important.  #defines are evil, and should only be used to control 
conditional compilation and other preprocesser-level direction, not 
actual code (or constants), IMHO.

(Actually, it's not just "IMHO"... I highly recommend "Large Scale C++ 
Software Design" by John Lakos.  In it, one of his major design rules 
states "Avoid using preprocessor macros in header files except as 
include guards".  The reason?  The compiler has no way to detect 
symbol/name conflicts with preprocesser macros, because the 
preprocesser has already done the textual replacement by the time the 
compiler runs.  Thus subtle compile or runtime errors can be introduced 
that can take an eternity of 'grep'-ing to track down.  The X11 headers 
are particularly guilty of this, defining macros like "#define None 
0L", which causes real problems if you want to define an enumeration 
like "enum CountType { None, One, Two, Three }" later on).

-JAC