Re: This blows my mind.

Discussion in 'C Programming' started by ataru@nospam.cyberspace.org, Aug 20, 2003.

  1. Guest

    ext_u <> broke the eternal silence and spoke thus:

    > "Exercise 1-13 Write a program to print a histogram of the lengths of
    > words in its input. It is easy to draw the histogram with the bars
    > horizontal; a vertical orientation is more challenging."


    Because I'm bored, here is a probably very poor attempt...

    int main(int argc, char *argv[])
    {
    int i, j;
    int longest=0;
    int cur_length;
    int *lengths;
    int *tmp;

    if ( !(lengths=malloc(1)) ) exit(1);

    for(i=1;i < argc;i++) {
    if( (length=strlen(argv)) > longest ) {
    longest=length;
    if( (tmp=realloc(lengths,longest+1)) == NULL) exit(1);
    lengths=tmp;
    lengths[longest]=0;
    }
    lengths[length]++;
    }

    for(i=0;i < longest;i++) {
    for(j=0;j < lengths;j++)
    printf("#");
    printf("\n");
    }

    return 0;
    }

    Probably not what K&R (or sane people) would do... but it might work...

    --
    Christopher Benson-Manica | Jumonji giri, for honour.
    ataru(at)cyberspace.org |
     
    , Aug 20, 2003
    #1
    1. Advertising

  2. Guest

    broke the eternal silence and spoke thus:
    > Because I'm bored, here is a probably very poor attempt...


    Talk about poor :( It's nice to have a compiler...

    #include <stdio.h>

    int main(int argc, char * argv[])
    {
    int i, j;
    int longest = 0;
    int cur; /* damn variable names */
    int * lengths; /* holds the lengths for later */
    int * tmp;

    if( !(lengths=(int *)malloc(1)) ) exit(1);

    for( i = 1; i < argc; i++ )
    {
    if( (cur = strlen(argv)) > longest)
    {
    longest = cur;
    if( !(tmp = (int *)realloc(lengths, longest + 1)) ) exit(1);
    lengths = tmp;
    lengths[longest] = 0;
    }
    lengths[cur]++;
    }

    for( i = 0; i <= longest; i++ )
    {
    printf("%d ", i);
    for( j = 0; j < lengths; j++ )
    printf("#");
    printf("\n");
    }

    return 0;
    }

    [17:47:14][asuka@soryuu:~]> ./a.out a a a ab a ac aaa aa aaa aaaa aa b a c
    0
    1 #######
    2 ###
    3 ###
    4 #

    --
    Christopher Benson-Manica | Jumonji giri, for honour.
    ataru(at)cyberspace.org |
     
    , Aug 20, 2003
    #2
    1. Advertising

  3. Mike Wahler Guest

    <> wrote in message
    news:bi0qk0$hfu$...
    > broke the eternal silence and spoke thus:
    > > Because I'm bored, here is a probably very poor attempt...

    >
    > Talk about poor :( It's nice to have a compiler...


    Your code is still very poor, and imo will harm
    rather than help the OP. Your compiler might have helped
    you a bit, but you told it to shut up by casting
    the return values of 'malloc()' and 'realloc()'.

    >
    > #include <stdio.h>
    >
    > int main(int argc, char * argv[])
    > {
    > int i, j;
    > int longest = 0;
    > int cur; /* damn variable names */
    > int * lengths; /* holds the lengths for later */


    This is a pointer. It cannot represent a 'length',
    only a memory address.

    > int * tmp;
    >
    > if( !(lengths=(int *)malloc(1)) ) exit(1);


    You have failed to include the header which declares
    'malloc()'. This results in your incorrect casting
    of its return value to hide the fact that it will
    return an int, very probably corrupting the valid
    address it tried to return.

    And why allocate a single byte?

    If 'malloc()' fails, you abruptly terminate the
    program, giving the user no indication of what's
    wrong.

    The only standard, portable values for the argument
    to 'exit()' are the value zero, or one of the
    macros 'EXIT_SUCCESS' or 'EXIT_FAILURE'.

    >
    > for( i = 1; i < argc; i++ )
    > {
    > if( (cur = strlen(argv)) > longest)


    You have failed to include the header which declares
    'strlen()'. This again can corrupt the return value
    (which btw is type 'size_t', not 'int', as your
    failure to include the prototype will force the
    compiler to assume.

    > {
    > longest = cur;
    > if( !(tmp = (int *)realloc(lengths, longest + 1)) ) exit(1);


    Same problems here as with 'malloc()'.

    IMO it's poor practice to put the statement controlled by
    an 'if' on the same line. Use another, indented line.
    Much more readable.

    > lengths = tmp;
    > lengths[longest] = 0;
    > }
    > lengths[cur]++;
    > }
    >
    > for( i = 0; i <= longest; i++ )
    > {
    > printf("%d ", i);
    > for( j = 0; j < lengths; j++ )
    > printf("#");
    > printf("\n");
    > }
    >
    > return 0;
    > }
    >
    > [17:47:14][asuka@soryuu:~]> ./a.out a a a ab a ac aaa aa aaa aaaa aa b a c
    > 0
    > 1 #######
    > 2 ###
    > 3 ###
    > 4 #


    After reading OP's post, do you really think he'll
    understand any of this? He's on chapter ONE, for
    goodness sake! Do you think he knows what e.g.
    'malloc()' is?

    Your code is horribly broken anyway. I suggest you
    do a bit of reading in K&R yourself.

    -Mike
     
    Mike Wahler, Aug 20, 2003
    #3
  4. Guest

    Mike Wahler <> broke the eternal silence and spoke thus:

    > Your code is still very poor, and imo will harm
    > rather than help the OP. Your compiler might have helped
    > you a bit, but you told it to shut up by casting
    > the return values of 'malloc()' and 'realloc()'.


    Well, had I been smart and not hurrying I would have compiled -Wall.. :(

    > And why allocate a single byte?


    Um, because I was in a hurry, and I'm dumb...?

    malloc(sizeof(int));
    malloc((length + 1) * sizeof(int));

    > You have failed to include the header which declares
    > 'strlen()'. This again can corrupt the return value
    > (which btw is type 'size_t', not 'int', as your
    > failure to include the prototype will force the
    > compiler to assume.


    Isn't defaulting to int not part of the standard anymore? If it still is,
    why?

    > IMO it's poor practice to put the statement controlled by
    > an 'if' on the same line. Use another, indented line.
    > Much more readable.


    > After reading OP's post, do you really think he'll
    > understand any of this? He's on chapter ONE, for
    > goodness sake! Do you think he knows what e.g.
    > 'malloc()' is?


    I know he doesn't... I'm really looking forward to someone posting a "real"
    solution, because I just don't know how K&R could possibly want it done w/o
    malloc...

    > Your code is horribly broken anyway. I suggest you
    > do a bit of reading in K&R yourself.


    Indeed... (although I really do know a *bit* more than it seems, really...)

    --
    Christopher Benson-Manica | Jumonji giri, for honour.
    ataru(at)cyberspace.org |
     
    , Aug 20, 2003
    #4
  5. Guest

    Mike Wahler <> broke the eternal silence and spoke thus:

    > Why sizeof (int)? The program will read characters.
    > And why the +1? The program will not be creating
    > strings.


    I don't know.... I should really just abandon this thread, and post under a
    new name, so I might have a chance of not being taken for a moron...

    > No, it's not. But if you're coding by the new standard,
    > invoking undeclared functions is not allowed. Ya can't
    > have it both ways. :)


    Well, I *meant* to declare them, does that count for anything?? ;) I really
    would have liked the code not to compile, rather than ending up being
    (justly!) chastised for my stupidity here...

    > It's not. But many (if not most) folks are still using
    > C89 translators, since there doesn't seem to be much
    > impetus to widely implement C99, for whatever reasons.


    So, why does the program work at all? If I haven't declared these functions,
    how can calling them have any meaning? Why did such dreadful code have the
    indecency to appear to work fine? (don't say just to humiliate me lol)

    > I hope they don't. Handing out ready-made solutions helps
    > the learner not at all. Better he post his attempts, and
    > get help and corrections. That way he'll learn something.
    > Remember this is a learning exercise. I suspect they
    > intend the reader to simply define some arbitrary-size
    > array, e.g.


    > char text[1000];


    I guess... so next time I will a) do it the easy way, and b) not post my
    "solution" here unless I somehow regain some shreds of dignity ;)

    > I certainly hope so. But whether using C89 or C99, it's
    > still *always* wrong to cast the return value from
    > 'malloc()' or 'realloc()'.


    I *knew* something was fishy when I got warnings about pointer-integer
    conversion, but being stubborn, I was like "take THAT gcc!" without pondering
    that maybe the dumbass in the situation was me ;)

    > I hope I didn't offend you too much by 'ripping into'
    > your code like that. I just couldn't let it be. :)


    Oh no, it was totally justified!! I'm just utterly ashamed that after several
    years of coding C (and liking it!) that I could conceivably come up with code
    so dreadfully broken, and have the hubris to actually think it worked... ;)

    --
    Christopher Benson-Manica | Jumonji giri, for honour.
    ataru(at)cyberspace.org |
     
    , Aug 21, 2003
    #5
  6. Guest

    Mike Wahler <> broke the eternal silence and spoke thus:

    > Why sizeof (int)? The program will read characters.
    > And why the +1? The program will not be creating
    > strings.


    Oh, I remember now - I'm storing the *lengths* of the strings in a dynamic
    array, so I can print them later. So sizeof(int) is right in that case, isn't
    it?

    --
    Christopher Benson-Manica | Jumonji giri, for honour.
    ataru(at)cyberspace.org |
     
    , Aug 21, 2003
    #6
  7. <> wrote in message
    news:bi0sbo$hjv$...
    ....
    > ... I'm really looking forward to someone posting a "real"
    > solution, because I just don't know how K&R could possibly want it done

    w/o
    > malloc...

    ....
    > --
    > Christopher Benson-Manica | Jumonji giri, for honour.
    > ataru(at)cyberspace.org |


    --snip--

    #include <stdio.h>

    #define MAXLENGTH 10

    int main(int argc, char* argv[])
    {
    int c, length, i;
    int histogram[MAXLENGTH];
    int toolong;

    toolong = 0;
    for (length = 0; length < MAXLENGTH; length++)
    histogram[length] = 0;

    length = 0;
    while ((c = getchar()) != EOF) {
    if (c == ' ' || c == '\t' || c == '\n' || c == '\r') {
    if (length > 0) {
    length--;
    if (length < MAXLENGTH)
    histogram[length]++;
    else
    toolong++;
    length = 0;
    }
    }
    else {
    length++;
    }
    }

    for (length = 0; length < MAXLENGTH; length++) {
    printf("%d: ", length + 1);
    for (i = 0; i < histogram[length]; i++)
    printf("#");
    printf("\n");
    }

    printf("%d words were too long\n", toolong);

    return 0;
    }
    --snip--

    --
    Roger
     
    Roger Willcocks, Aug 21, 2003
    #7
  8. Groovy hepcat was jivin' on Wed, 20 Aug
    2003 20:39:22 +0000 (UTC) in comp.lang.c.
    Re: This blows my mind.'s a cool scene! Dig it!

    >ext_u <> broke the eternal silence and spoke thus:
    >
    >> "Exercise 1-13 Write a program to print a histogram of the lengths of
    >> words in its input. It is easy to draw the histogram with the bars
    >> horizontal; a vertical orientation is more challenging."

    >
    >Because I'm bored, here is a probably very poor attempt...


    Indeed it is.

    >int main(int argc, char *argv[])
    >{
    > int i, j;
    > int longest=0;
    > int cur_length;
    > int *lengths;
    > int *tmp;
    >
    > if ( !(lengths=malloc(1)) ) exit(1);
    >
    > for(i=1;i < argc;i++) {
    > if( (length=strlen(argv)) > longest ) {
    > longest=length;
    > if( (tmp=realloc(lengths,longest+1)) == NULL) exit(1);
    > lengths=tmp;
    > lengths[longest]=0;
    > }
    > lengths[length]++;
    > }
    >
    > for(i=0;i < longest;i++) {
    > for(j=0;j < lengths;j++)
    > printf("#");
    > printf("\n");
    > }
    >
    > return 0;
    >}
    >
    >Probably not what K&R (or sane people) would do...


    Definitely not what people who know C would do.

    > but it might work...


    Then again, it might not. Let's examine the diagnostic output you
    recieved when you tried compiling it, shall we? What's that? You
    didn't try compiling it? Whell then, let's examine the diagnostic
    output I recieved when I tried compiling it. (These diagnostic
    messages are not unexpected.)

    testing.c(9): Error! E1010: Type mismatch

    Since you have not told the compiler that malloc() returns a pointer
    to void (by including stdlib.h), it (if it conforms to C90) assumes
    that malloc() returns an int. Hence the type mismatch when you attempt
    to assign the return value to a pointer.

    testing.c(12): Error! E1011: Symbol 'length' has not been declared

    You apparently misspelled something here. I'm not sure what. Maybe
    cur_length. (See below.)

    testing.c(14): Error! E1010: Type mismatch

    Since you have not told the compiler that realloc() returns a
    pointer to void (eg., by including stdlib.h), it (if it conforms to
    C90) assumes that realloc() returns an int. Hence the type mismatch
    when you attempt to assign the return value to a pointer.

    testing.c(14): Error! E1011: Symbol 'NULL' has not been declared

    Since you have not declared NULL (eg., by including any of the
    headers in which it is declared), it does not exist.

    testing.c(5): Warning! W202: Symbol 'cur_length' has been defined, but
    not referenced

    This speaks for itself. You never used cur_length, though you
    declared it. I think perhaps you misspelled it when you used it.

    testing.c(14): Warning! W301: No prototype found for 'realloc'
    testing.c(9): Warning! W301: No prototype found for 'exit'
    testing.c(9): Warning! W301: No prototype found for 'malloc'
    testing.c(12): Warning! W301: No prototype found for 'strlen'
    testing.c(23): Warning! W301: No prototype found for 'printf'

    These all speak for themselves. You have provided no declarations
    for these functions (eg., by including the headers in which they are
    declared).
    Now, let's fix these problems. Insert the following lines at the
    beginning of the program's source code:

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

    Next, going on the assumption that "length" is supposed to be
    "cur_length", change all occurrences of "length" to "cur_length".
    Now my compiler happily compiles the code without a problem. But
    that doesn't mean it's good code. There are still many things wrong
    with it. Let's take a look at those now, shall we?
    For a start, mainly as a matter of style, it is better to return
    from main() rather than call exit(). But more importantly, you are
    passing a non-portable value to exit(). The only values that can
    portably be passed to exit() or returned from main() are 0,
    EXIT_SUCCESS and EXIT_FAILURE (the latter two defined in stdlib.h).
    Secondly (and this is a biggie), you are allocating one single,
    solitary byte for a pointer to int. When you go to store or retrieve
    an int in this allocated memory, the single byte may not be large
    enough to hold such a beastie. Result: undefined behaviour. Sure, you
    attempt to allocate more space later, but you never account for the
    size of an int in your malloc() and realloc() calls. This is a very
    serious problem.
    You seem to be taking input from the command line instead of stdin.
    I think the intention (of the exercise) is that input be taken from
    stdin. (It's chapter 1 of K&R, remember. Command line arguments have
    not been discussed yet.)
    If the realloc() call fails, you're not releasing the original block
    of memory. Thus, you have a possible memory leak on your hands.
    Remember, if realloc() fails, the original block is untouched. That's
    why you use an extra pointer when using realloc(), so you can still
    access (and deallocate) the original memory block.
    If cur_length is more than one greater than longest, then you need
    to set *all* newly created elements of the reallocated array, not just
    the end one, to a starting value (0). Otherwise, intervening values
    will be indeterminate.
    Why use printf() to inefficiently display a single character? Use
    putchar() instead, and avoid printf()'s format parsing and variable
    argument overhead.

    --

    Dig the even newer still, yet more improved, sig!

    http://alphalink.com.au/~phaywood/
    "Ain't I'm a dog?" - Ronny Self, Ain't I'm a Dog, written by G. Sherry & W. Walker.
    I know it's not "technically correct" English; but since when was rock & roll "technically correct"?
     
    Peter Shaggy Haywood, Aug 24, 2003
    #8
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Tom Beretvas

    Netframework 1.1 install blows up

    Tom Beretvas, Jul 21, 2005, in forum: ASP .Net
    Replies:
    3
    Views:
    694
    Tom Beretvas
    Aug 12, 2005
  2. Gary

    I.E. blows after running my app

    Gary, Apr 2, 2004, in forum: ASP .Net
    Replies:
    1
    Views:
    311
    Alvin Bruney [MVP]
    Apr 3, 2004
  3. Amy B
    Replies:
    2
    Views:
    559
    Guest
    Feb 25, 2005
  4. clintonG
    Replies:
    2
    Views:
    587
    clintonG
    Nov 11, 2005
  5. ext_u

    Re: This blows my mind.

    ext_u, Aug 21, 2003, in forum: C Programming
    Replies:
    4
    Views:
    385
Loading...

Share This Page