[mythtv] CI Progress
linux.news at bucksch.org
Mon Aug 4 01:08:13 EDT 2003
Kenneth Aafl°y wrote:
>Ben Bucksch wrote:
>> * 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?
>Can't understand why you 'mix and match' class and C functions
I just explained why. Please don't change the style (esp. if that
results 1000 line changes), unless you asked first and made sure that
it's OK or there's a good technical reason to do so. In this case, there
is a good technical reason *not* to use your change.
BTW: It's not a real C function, it uses e.g. references. It's just a
>Originally wanted to move it into DVBChannel, as it belongs there, basic
DVBSettings is not part of the class interface, it's part of the
implementation, same for FetchDVBTuningOptions.
>I was trying to move pid setting out of DVBChannel, it has no reference to tuning at all. Besides, whats the point of using a vector to store it when it actually will become a C array?
Because vectors are far more convient and saver to use (no fixed
boundaries, no boundary checking, no mem management etc.), and we need
it in other places as well, not just for ts_to_ps(). It's a very
unfortunate fact that I have to use C for ts_to_ps (lib boundary), not
something to extend through the whole code.
>> * Properties are lower case, without m_ (it makes no sense to only
>> have half the properties start with m_), methods are upppercase.
(But you added Channel and m_settings properties.)
>> * What is that "version"? Is that a replacement for "bool
>> pid_changed;". Why is the latter insufficient?
>So where do I unset it
When you are sure that you made all necessary changes. If you want to
keep your solution (it would make some sense, if you need to make
changes in several, independent functions), please change the name to
|settings_change| or similar and make a comment what it's intended for.
>> * Why did you change the way the PID processing works?
>Out of DVBChannel, planning on writing a new demux function, because I'll
>need it if I'm going to write a Table parser (EPG/PMT...).
I'll need a much more detailed explanation.
>Then comment before I get close to releasing, gave a chance last time and
>gave a chance this time, and not with one-liners explaining notting!
I don't understand. You know that I was on vacation. I just took one
hour to look over your patch. I tried to make comments as early as
possible, so that you don't invest time into stuff that will be
rejected. If you want me to hold back my comments until you're done,
that's even easier for me, costs me less time, but please understand
that I'll decline changes that I think are worse or no better than the
More information about the mythtv-dev