Re: Some patches

[Leonard den Ottolander <leonard den ottolander nl>]

Hello Roland,

On Tue, 2004-09-28 at 11:42, Roland Illig wrote:
> I have banned me from doing any CVS commits for one week to let you all 
> calm down from the stressful last month with me. I think I can take that 
> promise, but I need some help from you. Please review these patches and 
> commit them if you like.

I hope after this week you won't feel so stressed yourself from not
having committed anything that you start committing poorly tested and
undiscussed code changes again ;-> .

> The patch for src/slint.c let's me build mc with the option 
> --with-screen=slang. For any reason the Debian-unstable SLang library 
> does not contain the symbol _SLsys_input_pending, but it contains 
> SLang_input_pending, so the patch adjusts the function call.

A quick glance raises the question whether the test for SLANG_VERSION
will result in the correct symbols being used on every platform / for
every version. Debian's slang might not provide that symbol, but other
slangs might. If this is really Debian specific the main tree might not
be the proper place to fix your issue. This is where vendor patches are
for.

By the way, on FC1:
$ nm libslang-utf8.so.1.4.5.debug | grep _input_pending
000215b0 T SLang_input_pending
000116f0 T _SLsys_input_pending

and

$ nm libslang-utf8.so.1.4.5.debug | grep _getkey        
00021400 T SLang_getkey
00062884 B SLang_getkey_intr_hook
00033570 T SLkp_getkey
00033610 T SLkp_set_getkey_function
00011840 T _SLsys_getkey

However, I wouldn't know if these symbols are interchangeable.

And what's the use of the mc_main? Seems just like a wrapper that
doesn't add anything extra like error handling. And why do you place the
updated main.c in another patch? These seem to be related and should
thus be in the same (separate?) patch.

> The "named constant" patch replaces literal -1 with INVALID_OFFSET where 
> appropriate. I am not sure if the one line where INT_MAX is replaced 
> with INVALID_OFFSET is really correct. Please check.

Just my thought. Please check this yourself. You have access to the code
just as easily as anybody else. Otoh somebody might just have the answer
without looking :) . INT_MAX might actually be an incorrect value, but
you need to check the calling code for equations. And do we need the new
name for the variable (INVALID_OFFSET instead of EOF_offset)?

By the way, providing a diff -up would make it easier to spot in which
function the code change is introduced, without having to check the
code.

> The patch for src/file.c fixes the prototypes for some functions when mc 
> is configured --without-largefile.

Same applies as for the introduction of consts before. Check for
regressions by trying out at least some basic functions that might be
affected by this code change.

Leonard.

-- 
mount -t life -o ro /dev/dna /genetic/research