[mythtv] CI Progress
linux.news at bucksch.org
Mon Aug 4 00:20:43 EDT 2003
Kenneth Aafl°y wrote:
>Ben Bucksch wrote:
>>Kenneth Aafl°y wrote:
>>>The code is ugly, but I've included a diff against dvb* files in
>>>libmythtv, so you have the opportunity to comment.
>>Is this patch obsolete now? I hope so...
>Why, was it that bad?
Yes. You touched almost all lines in the dvbchannel.cpp, I couldn't see
why, I didn't like the new style, and I think you broke some features,
but I am not sure, because the patch was unreadable (too many changes).
>Anyway, here's what I was hoping would get into CVS.
* If you make heavy whitespace changes, please *at least* include a
diff -wu or better yet submit them with a separate patch. Same
with code style changes.
- : ChannelBase(parent),
+ : ChannelBase(parent), cardnum(aCardnum), use_ts(a_use_ts)
I think the new code is harder to read than the old code. Don't
make such changes.
* You made FetchDVBTuningOptions() a method of DVBChannel. This
requires to move the definition of DVBSettings into the header
file. I intentionally left DVBSettings in the cpp file, because
it's local to the implementation, may change often and I didn't
want it to be public and cause rebuilds. You don't need the
function outside of dvbchannel.cpp, and it doesn't need to access
protected DVBChannel vars, so why that change?
* Please try to avoid pointers and raw C arrays, use local vars,
references or vectors instead. For example:
o header: + DVBSettings* m_settings;
consturctor: + m_settings =
then tons of
- else if (option == "16") s.guard_interval =
+ else if (option == "16") s->guard_interval =
and the like. Why don't you just use a DVBSettings
m_settings; property? Or better yet, leave things as-is,
keeping DVBSettings as local variable and separating out the
PIDs, which you need elsewhere? DVBSettings was only
intended as a convient handle to pass the tuning parameters
between the DB-reading/parsing function and those which do
the tuning, it wasn't intended to keep every setting related
o + uint16_t pids[MAX_PIDS];
- vector_int pid;
* Properties are lower case, without m_ (it makes no sense to only
have half the properties start with m_), methods are upppercase.
* What is that "version"? Is that a replacement for "bool
pid_changed;". Why is the latter insufficient?
* Why did you change the way the PID processing works?
In general, don't rewrite the file unless you have a really good reason
to. Please justify every change. I can't make sense of your changes, and
some of what you changed goes against explicit decisions I made, without
any appearant reason.
> - cleaned up dvb*
That "clean up" is what I disagree with.
> - builds without USING_DVB
No, you just moved it to tv_rec.cpp.
More information about the mythtv-dev