neater code
Bill Cunningham
out. Turn all off bits on and vice versa. I think this code can be a little
neater and I found that a 2Kb file became 2 bytes. I don't know if the
system is reading the file data wrong because everything has been "flipped"
or what. I want to be able to flip everything back but I might have in doing
this lost data. Could this file be made to look a little neater with maybe a
while or two instead of the IF's ? I tried it and failed. Any suggestions?
#include <stdio.h>
#include <stdlib.h>
int main(int argc, char **argv)
{
FILE *fp, *fp0;
int o = 0;
int i = 0;
int fl = ~o;
if (argc != 3) {
fputs("flip usage error\n", stderr);
exit(1);
}
fp = fopen(argv[1], "rb");
fp0 = fopen(argv[2], "wb");
if ((i = getc(fp)) != EOF)
i = getc(fp);
if ((o = putc(fl, fp0)) != EOF)
o = putc(fl, fp0);
fclose(fp);
fclose(fp0);
exit(0);
}
Chad
ELF? You mean the men that wear green tights and help Santa Claus?
Lew Pitcher
> I wrote this small program to take an ELF format file and turn it
> inside out.
Actually, ELF format has nothing to do with it. Your program doesn't depend
on an ELF input at all.
> Turn all off bits on and vice versa.
If that was the intent, then you didn't think it through very well. You see,
that's /not/ what your program does, not even close.
> I think this code can be a little neater
Likely ;-) /All/ programs look untidy
> and I found that a 2Kb file became 2 bytes.
Understandable, given your program.
> I don't know if the system is reading the file data wrong because
> everything has been "flipped" or what.
No. You just wrote a 2 byte file. That's what your logic explicitly does. It
does not "flip bits" or transform an input file
> I want to be able to flip everything back but I might have in doing this
> lost data.
So long as you kept the original input file, you should be OK. If you ran
your program with input and output files being the same file, then you
killed yourself. The input file cannot be recovered from the output file.
> Could this file be made to look a little neater with maybe a while or two
> instead of the IF's ?
Very certainly
> I tried it and failed. Any suggestions?
Let's look at your code, shall we?
> #include <stdio.h>
> #include <stdlib.h>
>
> int main(int argc, char **argv)
> {
> FILE *fp, *fp0;
> int o = 0;
> int i = 0;
> int fl = ~o;
Since /o/ is already set to 0, then /fl/ is now set to ~0, or "all bits 1"
> if (argc != 3) {
> fputs("flip usage error\n", stderr);
> exit(1);
> }
OK. Adequate argument checking for a sample program.
> fp = fopen(argv[1], "rb");
What if the file named by argv[1] doesn't exist? Will fopen() return a
valid FILE*? What should your program do if the input file doesn't exist?
Should it continue, create the output file, and attempt to process it's
non-existant input?
> fp0 = fopen(argv[2], "wb");
What if the file named by argv[2] is the same as the file named by argv[1]?
A "w" mode will cause fopen() to truncate the named file, and that'll
delete any data in it. If argv[2] names the same file as argv[1], then
you've just killed all your input. What other situations might you want to
cover here? What if fopen() returns an error?
> if ((i = getc(fp)) != EOF)
You read a character from the input file,
save it in /i/,
test it to see if it equals the EOF indicator, and
execute the next line if it doesn't.
That's one byte read out of your input file
> i = getc(fp);
(If the first input byte was not EOF),
you read a (second) character from the input and save it in /i/, discarding
the first input byte.
That's now two bytes read from the input file.
> if ((o = putc(fl, fp0)) != EOF)
You write one byte of ~0 fron /fl/ to the output file,
save the results of the write (which can be either your original ~0 or
EOF) in /o/,
test the results to see if it equals the EOF indicator, and
execute the next line if the results were not EOF
That's one byte of ~0 written to your output file.
> o = putc(fl, fp0);
(If the first write succeeded)
you write a second byte of ~0 from /fl/ to the output file, saving the
results of the putc() in /o/ (discarding the previous value of /o/)
That's now two bytes written to the output file.
> fclose(fp);
You close the input file, after reading only two bytes.
> fclose(fp0);
You close the output file, after writing only two bytes.
> exit(0);
You terminate the execution of your program, with a returncode of 0
> }
Notice in the above analysis...
1) You don't read the entire input file. You only read two bytes from the
input.
2) You don't write any of the input (transformed or not) to the output file.
You write only two bytes (at most) of constant data, unrelated to the
contents of the input file.
3) You don't do any transformation on the data obtained from the input file.
4) You don't prevent your program from deleting all the contents of the
input file before processing it
--------------------------------------------------------------
For processing /all/ the input, you need a looping mechanism.
The body of the loop should perform your "complement" transformation on the
input byte, and then write the byte to the output. The loop control itself
should obtain the next input byte, and terminate when the input is EOF.
You want something like ...
while ((i = getc(fp)) != EOF) /* get a byte into /i/, stop on EOF */
{
fl = ~i; /* /fl/ is bitflipped /i/ */
if ((o = putc(fl, fp0)) == EOF) /* write /fl/ to output file */
{ /* if write returned EOF */
fputs("flip failed to write", stderr); /* then we have an error */
exit(2);
}
}
HTH
--
Lew Pitcher
Master Codewright & JOAT-in-training | Registered Linux User #112576
Me: http://pitcher.digitalfreehold.ca/ | Just Linux: http://justlinux.ca/
---------- Slackware - Because I know what I'm doing. ------
Richard Heathfield
> I wrote this small program to take an ELF format file and turn it inside
> out. Turn all off bits on and vice versa.
#include <stdio.h>
int main(int argc, char **argv)
{
if(argc == 3)
{
FILE *fpi = fopen(argv[1], "rb");
if(fpi != NULL)
{
FILE *fpo = fopen(argv[2], "wb");
if(fpo != NULL)
{
unsigned char chi = 0;
unsigned char cho = 0;
int ok = 1;
while(ok && fread(&chi, 1, 1, fpi) == 1)
{
cho = ~chi;
if(fwrite(&cho, 1, 1, fpo) != 1)
{
ok = 0;
/* report the error */
}
}
if(ferror(fpi))
{
/* report the error */
}
if(fclose(fpo) != 0)
{
/* report the error */
}
}
else
{
/* report the error */
}
if(fclose(fpi) != 0)
{
/* report the error */
}
}
else
{
/* report the error */
}
}
else
{
/* report the error */
}
return 0;
}
--
Richard Heathfield <http://www.cpax.org.uk>
Email: -http://www. +rjh@
"Usenet is a strange place" - dmr 29 July 1999
Sig line vacant - apply within
Andrea Laforgia
I agree that he should report all the errors; don't you think that
something like the following would be more readable?
void closeFile(FILE *fp) {
if (fclose(fp)!=0) {
reportTheError(...);
}
}
void exitWithError(...) {
...
exit(1);
}
void reportTheError(...) {
...
}
int main(int argc, char **argv) {
FILE *fpi, *fpo;
unsigned char c;
if (argc != 3)
exitWithError(...);
if (!(fpi = fopen(argv[1], "rb"))
exitWithError(...);
if (!(fpo = fopen(argv[2], "wb")) {
closeFile(fpi);
exitWithError(...);
}
while (fread(&c, 1, 1, fpi)==1) {
unsigned char not_c = ~c;
if (fwrite(¬_c, 1, 1, fpo)!=1) {
reportTheError(...);
break;
}
}
if (ferror(fpi)) {
reportTheError(...);
}
closeFile(fpo);
closeFile(fpi);
return 0;
}
Keith Thompson
The real problem is that you think the problem with your code is
neatness rather than correctness.
A while loop is not "neater" than an if statement; it's a different
statement that does a very different thing. You don't choose to
use one or the other based on esthetics; you choose based on which
one does the job you want to do.
Your problem description is "to take an ELF format file and turn
it inside out. Turn all off bits on and vice versa."
What does the format of the input file (ELF or otherwise) have to
do with with this? ELF is an object file format. Inverting all
the bits in an ELF file gives you garbage.
Assuming you change "an ELF format file" to "a file", you're
left with something that might be a reasonable problem statement.
There might even be good reasons to generate a bit-flipped copy of
a given file; it might be a quick and dirty way to render a file
unusable while letting it be easily restored by inverting it again.
And as a simple programming exercise, it's not bad.
The big problem is that the program you posted doesn't do anything
resembling what you described.
The only value you ever write to the output file is fl. The value
of fl never changes after it's been initialized, and you only write
it (at most) twice.
There must have been a long chain of misconceptions leading
to the code you posted. One link in that chain was probably
a misunderstanding of the difference between "if" and "while".
Another *might* have been an assumption that, once you initialize
fl to ~o, it will remain equal to ~o as the value of o changes,
but that's only a guess on my part.
You're doing the equivalent of asking a driving instructor whether
you're using your turn signals properly, when the real problem is
that you need to stop driving the wrong way on the sidewalk.
--
Keith Thompson (The_Other_Keith) ks...@mib.org <http://www.ghoti.net/~kst>
Nokia
"We must do something. This is something. Therefore, we must do this."
-- Antony Jay and Jonathan Lynn, "Yes Minister"
Michael Foukarakis
with:
#include <stdio.h> /* Skip unnecessary includes for brevity */
int main(int argc, char **argv) /* to hell with the brevity */
{
while(printf("Hello world!\n") < 0)
printf("Error greeting world, please advise!\n");
return(~0); /* Flipping bits since ANSI C */
}
You can clearly see the part where the bits are reversed, the program
also prints a greeting for general awesomeness.
Barry Schwarz
<a...@andrealaforgia.it.invalid> wrote:
>
>I agree that he should report all the errors; don't you think that
>something like the following would be more readable?
>
>void closeFile(FILE *fp) {
> if (fclose(fp)!=0) {
> reportTheError(...);
How does reportTheError tell the difference between the input file and
the output file? How does closeFile inform the calling function that
an error occurred.?
> }
>}
>
>void exitWithError(...) {
> ...
> exit(1);
Why not use a portable return value?
>}
>
>void reportTheError(...) {
> ...
>}
>
>int main(int argc, char **argv) {
>
> FILE *fpi, *fpo;
> unsigned char c;
>
> if (argc != 3)
> exitWithError(...);
>
> if (!(fpi = fopen(argv[1], "rb"))
> exitWithError(...);
>
> if (!(fpo = fopen(argv[2], "wb")) {
> closeFile(fpi);
> exitWithError(...);
> }
>
> while (fread(&c, 1, 1, fpi)==1) {
> unsigned char not_c = ~c;
> if (fwrite(¬_c, 1, 1, fpo)!=1) {
> reportTheError(...);
> break;
> }
> }
>
> if (ferror(fpi)) {
> reportTheError(...);
> }
>
> closeFile(fpo);
> closeFile(fpi);
>
> return 0;
>}
--
Remove del for email
Bill Cunningham
"Michael Foukarakis" <electr...@gmail.com> wrote in message
news:1ec6bcad-d0b1-4938...@l11g2000yqb.googlegroups.com...
I tried rewriting your program in a neater way, here's what I came up
with:
#include <stdio.h> /* Skip unnecessary includes for brevity */
int main(int argc, char **argv) /* to hell with the brevity */
{
while(printf("Hello world!\n") < 0)
printf("Error greeting world, please advise!\n");
return(~0); /* Flipping bits since ANSI C */
}
You can clearly see the part where the bits are reversed, the program
also prints a greeting for general awesomeness.
Geesh that's much shorter. A simple return is all that's needed?
Bill
santosh
No. See Richard's post.
Keith Thompson
> "Michael Foukarakis" <electr...@gmail.com> wrote in message
> news:1ec6bcad-d0b1-4938...@l11g2000yqb.googlegroups.com...
> return(~0); /* Flipping bits since ANSI C */
> }
>
> You can clearly see the part where the bits are reversed, the program
> also prints a greeting for general awesomeness.
>
> Geesh that's much shorter. A simple return is all that's needed?
It depends on what you're trying to do. For what you originally
claimed you were trying to do, it's laughably insufficient.
Andrea Laforgia
wrote:
>How does reportTheError tell the difference between the input file and
>the output file?
Correct. I guess closeFile() is not a very good idea :)
> How does closeFile inform the calling function that
>an error occurred.?
Well, I didn't think it should. I thought of it as a replacement of
the group of lines attempting to close the file and reporting the
error. Instead of repeating those lines, one could use closeFile(),
but I realize that one should also pass a good bunch of information
together with the file pointer, in order to have a reliable error log,
which might make this function very little worth using :)
>>void exitWithError(...) {
>> ...
>> exit(1);
>
>Why not use a portable return value?
Right: EXIT_FAILURE. I simply copied the initial approach.
Michael Foukarakis
> "Bill Cunningham" <nos...@nspam.invalid> writes:
> >news:1ec6bcad-d0b1-4938...@l11g2000yqb.googlegroups.com...
> [...]
> > return(~0); /* Flipping bits since ANSI C */
> > }
>
> > You can clearly see the part where the bits are reversed, the program
> > also prints a greeting for general awesomeness.
>
> > Geesh that's much shorter. A simple return is all that's needed?
>
> It depends on what you're trying to do. For what you originally
> claimed you were trying to do, it's laughably insufficient.
But I wasn't trying to do that, I was simply going for a neater
version of its code. It turned out that lots of functionality could be
stripped leading to a much neater, visually speaking, program.
Nick Keighley
wrote:
> I agree that he should report all the errors; don't you think that
> something like the following would be more readable?
I doubt it! Richard heathfield is a structured programmer of the old
school.
"programs shall be written using only the constructs sequence,
iteration and conditional"
"each code block and function shall have a single entry point and a
single exit point"
no gotos no early exits
> void closeFile(FILE *fp) {
> if (fclose(fp)!=0) {
> reportTheError(...);
> }
>
> }
>
> void exitWithError(...) {
> ...
> exit(1);
good grief. You are suggesting Richard should invoke implementaion
defined behaviour when there is a well defined portable solution?!
:-)
> }
>
> void reportTheError(...) {
> ...
>
> }
>
> int main(int argc, char **argv) {
>
> FILE *fpi, *fpo;
> unsigned char c;
>
> if (argc != 3)
> exitWithError(...);
>
> if (!(fpi = fopen(argv[1], "rb"))
> exitWithError(...);
>
> if (!(fpo = fopen(argv[2], "wb")) {
> closeFile(fpi);
> exitWithError(...);
> }
>
> while (fread(&c, 1, 1, fpi)==1) {
> unsigned char not_c = ~c;
> if (fwrite(¬_c, 1, 1, fpo)!=1) {
> reportTheError(...);
> break;
> }
> }
>
> if (ferror(fpi)) {
> reportTheError(...);
> }
>
> closeFile(fpo);
> closeFile(fpi);
>
> return 0;
>
>
>
> }
--
It's probably not the quickest method around, but it uses plenty of
trees, and that's what counts.
Richard Heathfield
_JusSx_
#v+
#include <stdio.h>
#include <stdlib.h>
int
main (argc, argv)
int argc;
char *argv[];
{
int c;
while ((c = fgetc(stdin)) != EOF)
fputc(~c, stdout);
return EXIT_SUCCESS;
}
#v-
do you like this way?
-jussx-
--
Linux is only free if your time has no value
Ian Collins
>
> #v+
Eh?
> #include <stdio.h>
> #include <stdlib.h>
>
>
> int
> main (argc, argv)
> int argc;
> char *argv[];
Yuck, that's ancient K&R style. Avoid at all costs!
--
Ian Collins