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
_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://mythtv.org/cgi-bin/mailman/listinfo/mythtv-dev
