«

On Dec 26, 2:49*pm, j...@toerring.de (Jens Thoms Toerring) wrote:
> Maxx <grungeddd.m...@gmail.com> wrote:
> > Ok so i need to use fwrit() to write raw bytes, got it. Actually i
> > thought fread()/fwrite() were used to read large blocks of file.
>
> You can use them to read/write as many bytes as you need.
> They work for single bytes as well as for gigabytes. Of
> course, each call of such a function takes a bit of time,
> so it may be more efficient to read in larger chunks and
> thus, in the long run, one probably might be looking for
> a way to reduce the number of calls - but only after the
> algorithm that's applied to the data has been tested and
> proven to be working correctly. Try to get it right first
> and only then attempt to optimize
>
> Actually, if you write your program to have two func-
> tions, one for dealing with input and the other with
> output, like for example
>
> * int get_a_char( unsigned char * c );
>
> and
>
> * void output_rle_data( unsigned char count,
> * * * * * * * * * * * * unsigned char value );
>
> you can use them within your program to get new fresh bytes
> on which you apply your encoding algorithm and then to send
> out a new set of a count and a value, regardless of how
> exactly this is done.
>
> In a first version you might read just single bytes from
> a file at a time with get_a_char() and directly output
> encoded data to another file with output_rle_data(). I.e.
> get_a_char() at first could be as simple as
>
> int get_a_char( unsigned char *c )
> {
> * * * * return fread( c, 1, 1, fp );
>
> }
>
> In the caller (i.e. the part that does the run-length
> encoding) you check if the return value is 1 and then
> you know you got a new byte to deal with, otherwise
> you have reached the end of the file (or a read error
> occured).
>
> If you later want to get more fancy you can change just
> this function so that it reads in data in larger chunks.
> You could e.g make that function store them in a local
> buffer and return single bytes from that buffer to the
> caller, reading more from the file only when the buffer
> becomes empty. But the "interface" for the caller stays
> exactly the same as before, i.e. the caller continues to
> receive a new byte via the pointer argument as long as
> the return value isn't 0.
>
> The advantage is that you then don't have to change
> anything about your "core algorithm" that deals with
> the run-length encoding and which you know works cor-
> rectly. So any new bugs must be in the input function
> which makes it lot easier to debug the program. As an
> additional benefit it also will be easier for others
> to understand the program (and also for yourself when
> you read it again in half a years time).
>
> The same, of course, holds for the output function.
>
> * * * * * * * * * * * *Regards, Jens
> --
> * \ * Jens Thoms Toerring *___ * * *j...@toerring.de
> * *\__________________________ * * *http://toerring.de


I have rewritten the program using the guidelines that you've
provided, and it compiled well it didn't gave any runtime errors, but
the funny thing that happened is that the size of the resultant have
grown exactly twice. here is the program::

#include <stdio.h>
#include <stdlib.h>

#define MAXBUFF 5000
FILE *fp,*gp;
int get_a_char(unsigned char *);
void output_rle_data(unsigned char, unsigned char);

int main(int argc, char *argv[])
{

int n=0,i,m,j,k;
char name[20];
unsigned char c,count,a[MAXBUFF],*p=a;
sprintf(name,"%s.rle",argv[1]);
if(argc ==1)
{
fprintf(stderr,"Too few arguments");
exit(1);
}
if((fp=fopen(*++argv,"rb")) ==NULL)
{
fprintf(stderr,"Can't open file",*argv);
exit(2);
}

if((gp=fopen(name,"wb"))==NULL)
{
fprintf(stderr,"Can't open file");
exit(2);
}
while((m=get_a_char(&c)) ==1)
{
*p++=c;
++n;
}*p='\0';

for(i=0;i<n;i++)
{
j=a[i],count=0;
for(k=0;k<n;k++)
{
if(j=a[k])
++count;
}
output_rle_data(count,j);
}
}

int get_a_char(unsigned char *c)
{
return fread(&c, 1, 1, fp);
}

void output_rle_data(unsigned char cn, unsigned char jn)
{
fwrite(&cn,1,1,gp);
fwrite(&jn,1,1,gp);
}

On Mon, 27 Dec 2010 07:41:39 -0800 (PST), Maxx
<> wrote:

snip

>I have rewritten the program using the guidelines that you've
>provided, and it compiled well it didn't gave any runtime errors, but

It still produces the wrong result. Not all errors are caught by the
system.

>the funny thing that happened is that the size of the resultant have
>grown exactly twice. here is the program::

Since you write two bytes to your output file for every byte of your
input file, this is the only acceptable result. What were you
expecting?

>
>#include <stdio.h>
>#include <stdlib.h>
>
>#define MAXBUFF 5000
>FILE *fp,*gp;
>int get_a_char(unsigned char *);
>void output_rle_data(unsigned char, unsigned char);
>
>int main(int argc, char *argv[])
>{
>
> int n=0,i,m,j,k;
> char name[20];
> unsigned char c,count,a[MAXBUFF],*p=a;
> sprintf(name,"%s.rle",argv[1]);

How do you insure that the user never provides a command line
parameter longer than 15 characters? For that matter, how do you
insure that argv[1] exists and is not NULL?

> if(argc ==1)
> {

If you reach this point, the previous call to sprintf has already
invoked undefined behavior.

> fprintf(stderr,"Too few arguments");
> exit(1);

1 is not a portable return value from main. Use EXIT_FAILURE which
is.

> }
> if((fp=fopen(*++argv,"rb")) ==NULL)
> {
> fprintf(stderr,"Can't open file",*argv);

What purpose do you think the third argument serves. fprintf will
never look at it. Did you mean to include a %s in your format string?

> exit(2);

2 is not portable either.

> }
>
> if((gp=fopen(name,"wb"))==NULL)
> {
> fprintf(stderr,"Can't open file");

Wouldn't it be nice if you told the user which file could not be
opened?

> exit(2);
> }
> while((m=get_a_char(&c)) ==1)
> {
> *p++=c;

Due to an error in get_a_char, c is not assigned the character read
from the file. c is not initialized or assigned to so its value is
indeterminate. Your array a will be populated with garbage.

After you fix get_a_char, you still need to insure you do not overrun
the array a.

> ++n;
> }*p='\0';

To avoid confusion, this assignment should be on a new line.

>
> for(i=0;i<n;i++)
> {
> j=a[i],count=0;
> for(k=0;k<n;k++)
> {
> if(j=a[k])
> ++count;

Indenting consistently has one of the best cost/benefit ratios of all
style advice. You would also be better off not using tabs in posted
code.

Why are you using an unsigned char to count occurrences. Your array
holds 5000 elements. Surely one value can occur more than 255 times.

> }
> output_rle_data(count,j);

You are aware that if a value appears multiple times in the array, you
will output its count each time.

> }

main() returns an int and you should do so explicitly.

>}
>
>int get_a_char(unsigned char *c)
>{
> return fread(&c, 1, 1, fp);

While this cannot produce a diagnostic because the first parameter to
fread is a void* and therefore any pointer argument is valid, you have
provided the wrong value. You wanted fread to store the character
read from the file into the char whose address you passed in c. That
would be the unsigned char c defined in main. By using the same name
here, you managed to confuse yourself. The argument you actually pass
to fread is the address of the local pointer in this function. As a
result, fread will store the character read from the file in the first
byte of this pointer and not in the char defined in main. Delete the
&.

>}
>
>void output_rle_data(unsigned char cn, unsigned char jn)
>{
> fwrite(&cn,1,1,gp);
> fwrite(&jn,1,1,gp);
>}

--
Remove del for email

Maxx <> wrote:
> I have rewritten the program using the guidelines that you've
> provided, and it compiled well it didn't gave any runtime errors, but
> the funny thing that happened is that the size of the resultant have
> grown exactly twice. here is the program::

> #include <stdio.h>
> #include <stdlib.h>

> #define MAXBUFF 5000
> FILE *fp,*gp;
> int get_a_char(unsigned char *);
> void output_rle_data(unsigned char, unsigned char);

> int main(int argc, char *argv[])
> {

> int n=0,i,m,j,k;
> char name[20];

That may be much too short!

> unsigned char c,count,a[MAXBUFF],*p=a;
> sprintf(name,"%s.rle",argv[1]);

If the file name passed as the argument is has more than
15 characters thos will write past the end of the 'name'
array. Since you can't forsee how long the file name the
user inputs is going to be using a static buffer is rather
problematic - you would have to abbreiviate it if it's
too long (which might result in surprises for the user.
Better use a dynamically allocated buffer.

> if(argc ==1)
> {
> fprintf(stderr,"Too few arguments");
> exit(1);

Doing a return here will get you out also. And, while you're
free to return any integer value in principle, the only values
guaranteed to work everywere are EXIT_SUCESS and EXIT_FAILURE.

> }
> if((fp=fopen(*++argv,"rb")) ==NULL)

Why the strange "gymnastics" with the poor argv pointer?

> {
> fprintf(stderr,"Can't open file",*argv);
> exit(2);
> }

> if((gp=fopen(name,"wb"))==NULL)
> {
> fprintf(stderr,"Can't open file");
> exit(2);
> }
> while((m=get_a_char(&c)) ==1)
> {
> *p++=c;
> ++n;
> }*p='\0';

This is again very problematic. Consider what will happen if
the input file is longer than MAXBUFF-1: you will write past
the end of the 'a' array! And putting a '\0' in the last after
the last element is absolutely useless - this isn't a string.

Also, what I proposed was not to use any buffers in a first
version of the program. And if you use buffers it's not very
clever to use them in a way that restricts the possible size
of the files you can deal with. But if you do that you then
must check for longer files and find some way to deal with
the situation...

> for(i=0;i<n;i++)
> {
> j=a[i],count=0;
> for(k=0;k<n;k++)
> {
> if(j=a[k])
> ++count;
> }
> output_rle_data(count,j);
> }

Well, what you do here ins't run-length encoding at all. What
you do is counting for each position in the file how many bytes
exist in the whole file with the same value as at that position.
And then you write out the count and the value. Of course, the
resulting file will be twice as large as the input file. (And
the count may be wrong for a number of positions when the counter
overflowed).

Run-length encoding, as you described it yourself in your first
post, means something different: you count how many bytes of
the same value are in the file in the file, write that out and
then go on with the next set of bytes of the same value.

And then you're missing to return an int at the end

> }

> int get_a_char(unsigned char *c)
> {
> return fread(&c, 1, 1, fp);

Since you pass a pointer to the function this must be

return fread( c, 1, 1, fp );

Note the removed '&' in front of 'c'.

> }

> void output_rle_data(unsigned char cn, unsigned char jn)
> {
> fwrite(&cn,1,1,gp);
> fwrite(&jn,1,1,gp);
> }

Here's a version ofhow I might do it:

--------------8<----------------------------------------
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

static FILE *in_fp,
*out_fp;

static void rle_encode( void );
static int get_a_byte( unsigned char * );
static void output_rle_data( unsigned char,
unsigned char );

int main( int argc,
char * argv[ ] )
{
char *name;

if ( argc < 2 )
{
fprintf( stderr, "Too few arguments\n" );
return EXIT_FAILURE;
}

if ( ( in_fp = fopen( argv[ 1 ], "rb" ) ) == NULL )
{

fprintf( stderr, "Can't open input file '%s'\n", argv[ 1 ] );
return EXIT_FAILURE;
}

if ( ( name = malloc( strlen( argv[ 1 ] ) + 5 ) ) == NULL )
{
fprintf( stderr, "Running out of memory\n" );
fclose( in_fp );
return EXIT_FAILURE;
}

sprintf( name, "%s.rle", argv[ 1 ] );

if ( ( out_fp = fopen( name, "wb" ) ) == NULL )
{
fprintf( stderr,"Can't open output file '%s'\n", name );
free( name );
fclose( in_fp );
return EXIT_FAILURE;
}

free( name );

rle_encode( );

fclose( out_fp );
fclose( in_fp );
return EXIT_SUCCESS;
}

static void
rle_encode( void )
{
unsigned char count = 1,
curr,
next = 0;
int res;

/* Get the very first byte from the file */

res = get_a_byte( &curr );

/* Keep going while something could be read in */

while ( res != 0 )
{
/* Get a new byte from the file - stop if we already read 255 bytes
of the same value, if nothing could be read anymore or if the new
bytes value differs from the old one */

while ( count < 255 && ( res = get_a_byte( &next ) ) && next == curr )
count++;

/* Store the count and the value */

output_rle_data( count, curr );

/* Prepare for the next round with the byte we've just read in */

curr = next;
count = 1;
}
}

static int
get_a_byte( unsigned char * c )
{
return fread( c, 1, 1, in_fp );
}

static void
output_rle_data( unsigned char count,
unsigned char value )
{
fwrite( &count, 1, 1, out_fp );
fwrite( &value, 1, 1, out_fp );
}
--------------8<----------------------------------------

With this we have four functional units in the program - in main()
initialization is done (opening files etc.) and then the second
unit for doing the run-length encoding is called. That in turn
uses one unit for dealing with input and one for output.

Now, if you want to get more fancy and use buffers etc. you can
change the input and output functions to do that. For example,
we could change get_a_byte() to do instead

#define BUFFSIZE 5000

static int
get_a_byte( unsigned char * c )
{
static unsigned char buf[ BUFFSIZE ];
static size_t left_in_buf = 0,
next_in_buf;

if ( left_in_buf == 0 )
{
if ( ( left_in_buf = fread( buf, 1, BUFFSIZE, in_fp ) ) == 0 )
return 0;
next_in_buf = 0;
}

left_in_buf--;
*c = buf[ next_in_buf++ ];
return 1;
}

That way you read in a larger chunk of the input file at once
into a buffer and return bytes from that buffer. If the buffer
becomes empty you just read in one more chunk until the whole
file has been read in. All this is completely transparent to
the rest of the program and works with files of all sizes. The
function for doing the output can be modified in a similar way,
of course.
Regards, Jens
--
\ Jens Thoms Toerring ___
\__________________________ http://toerring.de

Maxx <> writes:

Just another matter that seems to have been missed:

> for(i=0;i<n;i++)
> {
> j=a[i],count=0;
> for(k=0;k<n;k++)
> {
> if(j=a[k])

This is not a test for j == a[k], it is an assignment.

> ++count;
> }
> output_rle_data(count,j);
> }

<snip>
--
Ben.

Jens Thoms Toerring <> wrote:
I didn't treat the case that there are 255 or more of the same
bytes in a row correctly, so two corrections must be made:

> static void
> rle_encode( void )
> {
> unsigned char count = 1,
> curr,
> next = 0;
> int res;
>
> /* Get the very first byte from the file */
>
> res = get_a_byte( &curr );
>
> /* Keep going while something could be read in */
>
> while ( res != 0 )
> {
> /* Get a new byte from the file - stop if we already read 255 bytes
> of the same value, if nothing could be read anymore or if the new
> bytes value differs from the old one */

> while ( count < 255 && ( res = get_a_byte( &next ) ) && next == curr )
> count++;
>
> /* Store the count and the value */
>

Insert here

if ( count > 0 )
> output_rle_data( count, curr );
>
> /* Prepare for the next round with the byte we've just read in */
>
> curr = next;
> count = 1;

Make that instead

count = count == 255 ? 0 : 1

> }
> }
Regards, Jens
--
\ Jens Thoms Toerring ___
\__________________________ http://toerring.de

On 28 Dec 2010 00:17:13 GMT, (Jens Thoms Toerring)
wrote:

>Jens Thoms Toerring <> wrote:
>I didn't treat the case that there are 255 or more of the same
>bytes in a row correctly, so two corrections must be made:
>
>> static void
>> rle_encode( void )
>> {
>> unsigned char count = 1,
>> curr,
>> next = 0;
>> int res;
>>
>> /* Get the very first byte from the file */
>>
>> res = get_a_byte( &curr );
>>
>> /* Keep going while something could be read in */
>>
>> while ( res != 0 )
>> {
>> /* Get a new byte from the file - stop if we already read 255 bytes
>> of the same value, if nothing could be read anymore or if the new
>> bytes value differs from the old one */
>
>> while ( count < 255 && ( res = get_a_byte( &next ) ) && next == curr )
>> count++;

This no longer computes the letter distribution but counts consecutive
letters. Maybe you should first decide what the intent is.

>>
>> /* Store the count and the value */
>>
>
>Insert here
>
> if ( count > 0 )
>> output_rle_data( count, curr );
>>
>> /* Prepare for the next round with the byte we've just read in */
>>
>> curr = next;
>> count = 1;
>
>Make that instead
>
> count = count == 255 ? 0 : 1
>
>> }
>> }
> Regards, Jens

--
Remove del for email

Barry Schwarz <> wrote:
> This no longer computes the letter distribution but counts consecutive
> letters. Maybe you should first decide what the intent is.

Maxx wrote in his first post:

|| This technique reduces the size of the file by replacing the
|| original bytes with the repetition count and the byte to be repeated.
|| Ex: if the file to be compressed starts with the following sequence of
|| bytes::
|| 46 6F 6F 6F 2B 2B 9C 81 81 81 81 84 84 84 7A 7A 7A
||
|| The compressed file will contain the following bytes::
|| 01 46 03 6F 02 2B 01 9C 04 81 03 84 03 7A

That looks to me like he's jsut trying to count consecutive
characters, doesn't it?
Regards, Jens
--
\ Jens Thoms Toerring ___
\__________________________ http://toerring.de

On Dec 27, 10:26*am, Barry Schwarz <schwa...@dqel.com> wrote:
> On Mon, 27 Dec 2010 07:41:39 -0800 (PST), Maxx
>
> <grungeddd.m...@gmail.com> wrote:
>
> snip
>
> >I have rewritten the program using the guidelines that you've
> >provided, and it compiled well it didn't gave *any runtime errors, but
>
> It still produces the wrong result. *Not all errors are caught by the
> system.
>
> >the funny thing that happened is that the size of the resultant have
> >grown exactly twice. here is the program::
>
> Since you write two bytes to your output file for every byte of your
> input file, this is the only acceptable result. *What were you
> expecting?
>
>
>
>
>
> >#include <stdio.h>
> >#include <stdlib.h>
>
> >#define MAXBUFF * * 5000
> >FILE *fp,*gp;
> >int get_a_char(unsigned char *);
> >void output_rle_data(unsigned char, unsigned char);
>
> >int main(int argc, char *argv[])
> >{
>
> > * *int n=0,i,m,j,k;
> > * *char name[20];
> > * *unsigned char c,count,a[MAXBUFF],*p=a;
> > * *sprintf(name,"%s.rle",argv[1]);
>
> How do you insure that the user never provides a command line
> parameter longer than 15 characters? *For that matter, how do you
> insure that argv[1] exists and is not NULL?
>
> > * *if(argc ==1)
> > * *{
>
> If you reach this point, the previous call to sprintf has already
> invoked undefined behavior.
>
> > * * * * * *fprintf(stderr,"Too few arguments");
> > * * * * * *exit(1);
>
> 1 is not a portable return value from main. *Use EXIT_FAILURE which
> is.
>
> > * *}
> > * *if((fp=fopen(*++argv,"rb")) ==NULL)
> > * *{
> > * * * * * *fprintf(stderr,"Can't open file",*argv);
>
> What purpose do you think the third argument serves. *fprintf will
> never look at it. *Did you mean to include a %s in your format string?
>
> > * * * * * *exit(2);
>
> 2 is not portable either.
>
> > * *}
>
> > * *if((gp=fopen(name,"wb"))==NULL)
> > * *{
> > * * * * * *fprintf(stderr,"Can't open file");
>
> Wouldn't it be nice if you told the user which file could not be
> opened?
>
> > * * * * * *exit(2);
> > * *}
> > * *while((m=get_a_char(&c)) ==1)
> > * *{
> > * * * * * **p++=c;
>
> Due to an error in get_a_char, c is not assigned the character read
> from the file. *c is not initialized or assigned to so its value is
> indeterminate. *Your array a will be populated with garbage.
>
> After you fix get_a_char, you still need to insure you do not overrun
> the array a.
>
> > * * * * * *++n;
> > * *}*p='\0';
>
> To avoid confusion, this assignment should be on a new line.
>
>
>
> > * *for(i=0;i<n;i++)
> > * *{
> > * * * * * *j=a[i],count=0;
> > * * * * * *for(k=0;k<n;k++)
> > * * * * * *{
> > * * * * * * * * * *if(j=a[k])
> > * * * * * * * * * * * * * *++count;
>
> Indenting consistently has one of the best cost/benefit ratios of all
> style advice. *You would also be better off not using tabs in posted
> code.
>
> Why are you using an unsigned char to count occurrences. *Your array
> holds 5000 elements. *Surely one value can occur more than 255 times.
>
> > * * * * * *}
> > * * * * * *output_rle_data(count,j);
>
> You are aware that if a value appears multiple times in the array, you
> will output its count each time.
>
> > * *}
>
> main() returns an int and you should do so explicitly.
>
> >}
>
> >int get_a_char(unsigned char *c)
> >{
> > * *return fread(&c, 1, 1, fp);
>
> While this cannot produce a diagnostic because the first parameter to
> fread is a void* and therefore any pointer argument is valid, you have
> provided the wrong value. *You wanted fread to store the character
> read from the file into the char whose address you passed in c. *That
> would be the unsigned char c defined in main. *By using the same name
> here, you managed to confuse yourself. *The argument you actually pass
> to fread is the address of the local pointer in this function. *As a
> result, fread will store the character read from the file in the first
> byte of this pointer and not in the char defined in main. *Delete the
> &.
>
> >}
>
> >void output_rle_data(unsigned char cn, unsigned char jn)
> >{
> > * *fwrite(&cn,1,1,gp);
> > * *fwrite(&jn,1,1,gp);
> >}
>
> --
> Remove del for email


Yeah still got it wrong. Well the program did opposite of what i
expected it to do.,,my bad..

Ah its just a test so i set the buffer to a random value of 20.

> What purpose do you think the third argument serves. fprintf will
> never look at it. Did you mean to include a %s in your format string?
>
Yeah i did meant to include a %s in the format string. another
mistake.I used unsigned char cause to avoid all that negative values.

> While this cannot produce a diagnostic because the first parameter to
> fread is a void* and therefore any pointer argument is valid, you have
> provided the wrong value. You wanted fread to store the character
> read from the file into the char whose address you passed in c. That
> would be the unsigned char c defined in main. By using the same name
> here, you managed to confuse yourself. The argument you actually pass
> to fread is the address of the local pointer in this function. As a
> result, fread will store the character read from the file in the first
> byte of this pointer and not in the char defined in main. Delete the
> &.

Totally didn't saw it coming.Thanks for pointing this out

On Dec 27, 11:41*am, j...@toerring.de (Jens Thoms Toerring) wrote:
> Maxx <grungeddd.m...@gmail.com> wrote:
> > I have rewritten the program using the guidelines that you've
> > provided, and it compiled well it didn't gave *any runtime errors, but
> > the funny thing that happened is that the size of the resultant have
> > grown exactly twice. here is the program::
> > #include <stdio.h>
> > #include <stdlib.h>
> > #define MAXBUFF 5000
> > FILE *fp,*gp;
> > int get_a_char(unsigned char *);
> > void output_rle_data(unsigned char, unsigned char);
> > int main(int argc, char *argv[])
> > {
> > * * * * int n=0,i,m,j,k;
> > * * * * char name[20];
>
> That may be much too short!
>
> > * * * * unsigned char c,count,a[MAXBUFF],*p=a;
> > * * * * sprintf(name,"%s.rle",argv[1]);
>
> If the file name passed as the argument is has more than
> 15 characters thos will write past the end of the 'name'
> array. Since you can't forsee how long the file name the
> user inputs is going to be using a static buffer is rather
> problematic - you would have to abbreiviate it if it's
> too long (which might result in surprises for the user.
> Better use a dynamically allocated buffer.
>
> > * * * * if(argc ==1)
> > * * * * {
> > * * * * * * * * fprintf(stderr,"Too few arguments");
> > * * * * * * * * exit(1);
>
> Doing a return here will get you out also. And, while you're
> free to return any integer value in principle, the only values
> guaranteed to work everywere are EXIT_SUCESS and EXIT_FAILURE.
>
> > * * * * }
> > * * * * if((fp=fopen(*++argv,"rb")) ==NULL)
>
> Why the strange "gymnastics" with the poor argv pointer?
>
> > * * * * {
> > * * * * * * * * fprintf(stderr,"Can't open file",*argv);
> > * * * * * * * * exit(2);
> > * * * * }
> > * * * * if((gp=fopen(name,"wb"))==NULL)
> > * * * * {
> > * * * * * * * * fprintf(stderr,"Can't open file");
> > * * * * * * * * exit(2);
> > * * * * }
> > * * * * while((m=get_a_char(&c)) ==1)
> > * * * * {
> > * * * * * * * * *p++=c;
> > * * * * * * * * ++n;
> > * * * * }*p='\0';
>
> This is again very problematic. Consider what will happen if
> the input file is longer than MAXBUFF-1: you will write past
> the end of the 'a' array! And putting a '\0' in the last after
> the last element is absolutely useless - this isn't a string.
>
> Also, what I proposed was not to use any buffers in a first
> version of the program. And if you use buffers it's not very
> clever to use them in a way that restricts the possible size
> of the files you can deal with. But if you do that you then
> must check for longer files and find some way to deal with
> the situation...
>
> > * * * * for(i=0;i<n;i++)
> > * * * * {
> > * * * * * * * * j=a[i],count=0;
> > * * * * * * * * for(k=0;k<n;k++)
> > * * * * * * * * {
> > * * * * * * * * * * * * if(j=a[k])
> > * * * * * * * * * * * * * * * * ++count;
> > * * * * * * * * }
> > * * * * * * * * output_rle_data(count,j);
> > * * * * }
>
> Well, what you do here ins't run-length encoding at all. What
> you do is counting for each position in the file how many bytes
> exist in the whole file with the same value as at that position.
> And then you write out the count and the value. Of course, the
> resulting file will be twice as large as the input file. (And
> the count may be wrong for a number of positions when the counter
> overflowed).
>
> Run-length encoding, as you described it yourself in your first
> post, means something different: you count how many bytes of
> the same value are in the file in the file, write that out and
> then go on with the next set of bytes of the same value.
>
> And then you're missing to return an int at the end
>
> > }
> > int get_a_char(unsigned char *c)
> > {
> > * * * * return fread(&c, 1, 1, fp);
>
> Since you pass a pointer to the function this must be
>
> * * * * *return fread( c, 1, 1, fp );
>
> Note the removed '&' in front of 'c'.
>
> > }
> > void output_rle_data(unsigned char cn, unsigned char jn)
> > {
> > * * * * fwrite(&cn,1,1,gp);
> > * * * * fwrite(&jn,1,1,gp);
> > }
>
> Here's a version ofhow I might do it:
>
> --------------8<----------------------------------------
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
>
> static FILE *in_fp,
> * * * * * * *out_fp;
>
> static void rle_encode( void );
> static int get_a_byte( unsigned char * );
> static void output_rle_data( unsigned char,
> * * * * * * * * * * * * * * *unsigned char );
>
> int main( int * *argc,
> * * * * * char * argv[ ] )
> {
> * * char *name;
>
> * * if ( argc < 2 )
> * * {
> * * * * fprintf( stderr, "Too few arguments\n" );
> * * * * return EXIT_FAILURE;
> * * }
>
> * * if ( ( in_fp = fopen( argv[ 1 ], "rb" ) ) == NULL )
> * * {
>
> * * * * fprintf( stderr, "Can't open input file '%s'\n", argv[ 1 ] );
> * * * * return EXIT_FAILURE;
> * * }
>
> * * if ( ( name = malloc( strlen( argv[ 1 ] ) + 5 ) ) == NULL )
> * * {
> * * * * fprintf( stderr, "Running out of memory\n" );
> * * * * fclose( in_fp );
> * * * * return EXIT_FAILURE;
> * * }
>
> * * sprintf( name, "%s.rle", argv[ 1 ] );
>
> * * if ( ( out_fp = fopen( name, "wb" ) ) == NULL )
> * * {
> * * * * fprintf( stderr,"Can't open output file '%s'\n", name );
> * * * * free( name );
> * * * * fclose( in_fp );
> * * * * return EXIT_FAILURE;
> * * }
>
> * * free( name );
>
> * * rle_encode( );
>
> * * fclose( out_fp );
> * * fclose( in_fp );
> * * return EXIT_SUCCESS;
>
> }
>
> static void
> rle_encode( void )
> {
> * * unsigned char count = 1,
> * * * * * * * * * curr,
> * * * * * * * * * next = 0;
> * * int res;
>
> * * /* Get the very first byte from the file */
>
> * * res = get_a_byte( &curr );
>
> * * /* Keep going while something could be read in */
>
> * * while ( res != 0 )
> * * {
> * * * * /* Get a new byte from the file - stop if we already read 255 bytes
> * * * * * *of the same value, if nothing could be read anymore or if the new
> * * * * * *bytes value differs from the old one */
>
> * * * * while ( count < 255 && ( res = get_a_byte( &next ) ) && next == curr )
> * * * * * * count++;
>
> * * * * /* Store the count and the value */
>
> * * * * output_rle_data( count, curr );
>
> * * * * /* Prepare for the next round with the byte we've just read in */
>
> * * * * curr = next;
> * * * * count = 1;
> * * }
>
> }
>
> static int
> get_a_byte( unsigned char * c )
> {
> * * return fread( c, 1, 1, in_fp );
>
> }
>
> static void
> output_rle_data( unsigned char count,
> * * * * * * * * *unsigned char value )
> {
> * * fwrite( &count, 1, 1, out_fp );
> * * fwrite( &value, 1, 1, out_fp );}
>
> --------------8<----------------------------------------
>
> With this we have four functional units in the program - in main()
> initialization is done (opening files etc.) and then the second
> unit for doing the run-length encoding is called. That in turn
> uses one unit for dealing with input and one for output.
>
> Now, if you want to get more fancy and use buffers etc. you can
> change the input and output functions to do that. For example,
> we could change get_a_byte() to do instead
>
> #define BUFFSIZE 5000
>
> static int
> get_a_byte( unsigned char * c )
> {
> * * static unsigned char buf[ BUFFSIZE ];
> * * static size_t left_in_buf = 0,
> * * * * * * * * * next_in_buf;
>
> * * if ( left_in_buf == 0 )
> * * {
> * * * * if ( ( left_in_buf = fread( buf, 1, BUFFSIZE, in_fp ) ) == 0 )
> * * * * * * return 0;
> * * * * next_in_buf = 0;
> * * }
>
> * * left_in_buf--;
> * * *c = buf[ next_in_buf++ ];
> * * return 1;
>
> }
>
> That way you read in a larger chunk of the input file at once
> into a buffer and return bytes from that buffer. If the buffer
> becomes empty you just read in one more chunk until the whole
> file has been read in. All this is completely transparent to
> the rest of the program and works with files of all sizes. The
> function for doing the output can be modified in a similar way,
> of course.
> * * * * * * * * * * * * * * Regards, Jens
> --
> * \ * Jens Thoms Toerring *___ * * *j...@toerring.de
> * *\__________________________ * * *http://toerring.de

As this program was just a test so i've used a random value of 20 as
my file name buffer, very immature of me.Ok now i've realized the
problems with the buffers, they always tend to bug me.
What i couldn't get my head around was the main alogrithm of the
program, previously i thought my method would give me the correct
result, i also did some trials with a few values and all that seem
fair, but i was completely missing the bigger picture, which i why my
program failed every time.
And thanks a lot for your program, it really cleared all my
doubts.Thanks

In article
<7ee9410d-39fd-4740-b6eb->,
James Dow Allen <> wrote:

> On Dec 24, 4:56Â*pm, Maxx <grungeddd.m...@gmail.com> wrote:
> > Â* Â* Â* Â* if(fp=fopen(*++argv,"rb")==NULL) /* bug */
> > [more code whose functionality, or lack thereof,
> > is irrelevant]
>
> My comment is intended not just for Maxx, but for
> Arnuld and a few others who post here.
>
> I've been programming for quite a while now, and make
> relatively few mistakes, yet I *STILL* will compile
> and run code before it's finished, just to do piecemeal
> verification. Perhaps I should be embarrassed to

I will compile with cc -c _before_ it can run.

> admit it, but sometimes I even have something like
> printf("Last arg is *%s*\n", argv[argc-1]);
> just to make sure I've not gone off-by-one somewhere.
> Yet we see complicated programs posted with the
> first line wrong!
>
> Long ago I worked in a 24-hour turnaround environment:
> code as much as you can accurately in a day, and
> study the coredump because *you only get one
> compile-and-go per day!* Those days are gone now.
> Use the compiler interactively and let it
> be your friend!

Uh huh. I get worried when there
are no compile errors or warnings.

--
Michael Press