OK, I plead guilty of lazyness and sloppy design in a few cases..
On Saturday 21 June 2008, Phil Dibowitz wrote:
> I have a request for future submissions of patches that have multiple
> parts (i.e. patchsets). I'd like to follow the general LKML standard
> here for this...
A valid point - I will follow that for my upcoming attempts.
> Libirremotes integration should be done in an entirely separate patch.
Another case of lazyness - even more as I did so for the first version
of my patch set, but didn't repeat for the current.
> LIBCONCORD LEARNIR API CHANGE
> * At the bottom of _get_next_key() you said you need to set the index
> and the name. You actually set to the index in the parent function
Yes, the comment is wrong, probably it was that way at some point,
until I decided that _get_next_key should only deal with the pointers,
and only get_key_name should care about the key counting.
> * It doesn't seem to me like previous_index and previous_name are
> properly named.
Agreed, 'current' would probably be better naming.
Following your and Stephen's other comments, all that will probably end
up quite different anyway...
> - if (search - data >= data_size) {
> + if (search >= data + data_size) {
> why change it? Especially given I think the former is clearer...
Justification was to get rid of a VC++ compiler warning that complained
about comparing data of different singnedness - apparently the
compiler saw that a difference might in theory become negative, so
it's signed, while data_size is unsgined. I prefer to fix compiler
warnings in the code, even if they may look redundant.
> LIBIRREMOTE
> * Why do your error codes in the opposite (numerical) direction? Are you
> trying to avoid conflict, or ...?
Looking at it now, it is a historical relict from the initial phase
where I wanted the scanf-functions to return some useful positve
value in case of success. Now that they ended up returning just
'OK' or some error, I will change the error codes to positive.
> * I really, really, really really dislike variables with capitols -
> ...Anyway, consider num_pulses, pulses, repeat_seq, etc.
Well, with Ada actually being my 'native' language, I'd rather prefer
Variables_With_Capial_Initals, but since I'm also maintaining lots
of legacy code from different authors, I may have become a little deaf
to consistent good coding style...
Anyway, the most weird looking names related to Pronto codes were initially
copied verbatim from the excellent document about the different Pronto IR
formats by Evgueni Oulianov ([EMAIL PROTECTED]), which can be found e.g at:
http://www.hifi-remote.com/infrared/prontoirformats.pdf
N.B.: I thought I had credited his work somewhere in the README, or at
least in the code, but apparently I missed that.
I see I have been quite inconsistent in style also in the implementation
of libIRremotes, and fixed that for the next version.
> CONCORDANCE
> ...I *would* like to (as I mentioned earlier), merge just the libconcord
> API change, let it bake in CVS for a bit, have people test it, and *then*
> merge the libirremote integration.
as you like it..
> * Your update to the man page.... Eh, I don't want that list of authors
> in the man page growing everytime someone adds a feature.
Never mind, just drop it.
> * Hmm - should be be showing pulses and details in non-verbose mode? I
> would think that _dump_new_code() should be wrapped in if
> ((*options).verbose).
It mostly is, but parts should possibly wrapped even deeper into
#ifdef _DEBUG, since they are mostly aimed at the developer (i.e. me).
I'd like to keep the ASCII plot, since it gives you an idea about what is going
on, though not as good as the actual plot in congruity,
and may help to detect problems in learning codes.
> I can't comment on it until I actually TRY it, which I can't do until I
> get my stuff, which is another month or so away.
> (hmm - feel like copy-n-pasting a session of use?)
Sounds like sending executables (Win32 or LINUX) will probably not be
helpful - but I'll have to get the Harmony out of my kids' hands first
for a sample session...
Andreas - more to come soon following the other comments --------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php
_______________________________________________
concordance-devel mailing list
concordance-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/concordance-devel
