[mythtv] CI Progress

Ben Bucksch 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 
normal function.

>Originally wanted to move it into DVBChannel, as it belongs there, basic
>classwriting.
>
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.
>>    
>>
>Agree!
>
(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 
current solution.



More information about the mythtv-dev mailing list