Thread: Advices about an exercise.

Write a program to "fold" long input lines into two or more shorter lines after the last non-blank character that occurs before the n-th column of input. Make sure your program does something intelligent with very long lines, and if there are no blanks or tabs before the specified column.

Okay, so i just did this one; with dynamic allocation and other nifty stuff which i find usefull (as detabing the line before folding).

I think this program can be really usefull if you have like huge and long lines in notepad for example or text editor and you want to to shorten it.

It is not limited by line size (uses dynamically allocated buffer).

So i'd like your opinion if there's something i missed/did wrong in my code, or any suggestions of how i could improve the code . I'm still learning so any advice or critisicm is welcome. Thanks!

Code:

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

#define FOLD_COLUMN 20

int last_word = 0;

int next_line_length()
{
    int c, i;
    
    i = 0;
    while((c = getchar()) != EOF && c != '\n')
       i++;
    
    if(i > 0 && c == EOF)
       last_word = 1;
    return (c == EOF && !i) ? EOF : i;
}

void getline(char *p)
{
     int c;
     
     while((c = getchar()) != EOF && c != '\n')
        *p++ = c;
     *p = '\0';
}

void fold_line(char *line, int len)
{
     int iterate_through_text(char *s, int pos);
     
     int i;
     int nl_pos = 0;    /* 1 position after the spot where we put newline */
     int no_blanks = 1; /* check if there are no blanks/tabs before
                           the specified column */
                           
     while(nl_pos + FOLD_COLUMN < len) { /* need not to check last FOLD_COLUMN characters */
         for(i = nl_pos + FOLD_COLUMN; i >= nl_pos; i--) {
             if(line[i] == ' ' || line[i] == '\t') {
                 line[i] = '\n';
                 nl_pos = i + 1;
                 no_blanks = 0;
                 break;
             }
         }
         if(no_blanks)
             nl_pos = iterate_through_text(line, nl_pos + FOLD_COLUMN);
         no_blanks = 1;
     }
}

int iterate_through_text(char *s, int pos)
{
    while(s[pos] != ' ' && s[pos] != '\t' && s[pos] != '\0')
       pos++;
    if(s[pos]) 
       s[pos] = '\n';
    return pos + 1;
}

#define TAB_STOP 8

int detab_length(char *s, int len)
{
     int i, column;
     int add_len;
     
     column = add_len = 0;
     for(i = 0; i < len; i++)
         if(s[i] == '\t') {
              add_len += TAB_STOP - (column % TAB_STOP);
              column = 0;
         } else
               column = (s[i] == '\n') ? 0 : column + 1;
               
     return add_len;
}

void detab(char *to, char *from, int len)
{
     int i, j, k;
     int column;
     
     column = j = 0;
     for(i = 0; i < len; i++)
         if(from[i] == '\t') {
              for(k = 0; k < TAB_STOP - (column % TAB_STOP); k++)
                 to[j++] = ' ';
              column = 0;
         } else {
               to[j++] = from[i];
               column = (from[i] == '\n') ? 0 : column + 1;
         }
     to[j] = '\0';
}
int main()
{
     int i, c;
     int alloc;            /* bytes needed to be allocated for the line */
     char *buf = NULL;     /* stores the line */
     int new_alloc;        /* bytes needed to be allocated for the detabed line */
     char *detabed = NULL; /* stores the line, but detabed */
     int pos = 0;          /* position in the file at which starts the next line */
     
     while((alloc = next_line_length()) != EOF) {
         buf = (char *) malloc(alloc + 1); /* alloc + 1 for the \0 character */
         fseek(stdin, pos, SEEK_SET);      /* go back to start + pos positions in the file */
         getline(buf);                     /* store the line */
         if((new_alloc = alloc + detab_length(buf, alloc)) != alloc) { /* detab it only if length changed */
            detabed = (char *) malloc(new_alloc + 1);
            detab(detabed, buf, alloc);
            fold_line(detabed, new_alloc);
            printf("%s", detabed);
         } else {
                fold_line(buf, alloc);
                printf("%s", buf);
         }
         pos += (alloc + 2);               /* increase pos by alloc + 2 (\n and next pos) */
         if(!last_word)
            putchar('\n');
     }
     return 0;
}

For example, do programmers usually check if fseek fails? Or allocation?

Should i add that?

Where are the frees? I see malloc, but I don't see free.

You should always check that your memory allocation worked, and add an abort call if it fails. Otherwise you will end up trying to use memory that isn't allocated and it will crash anyway.
for example, if you want an n-1 character string:

Code:

char *ptr;
if ((ptr = malloc(n*sizeof(char))) == NULL)
{
     printf("Memory allocation for ptr failed\n");
     abort();
}

probably you may want to read 'fold' source code.
man fold

There are other functions that are just as dangerous, too. Any function that writes into a buffer provided as a parameter MUST have a size argument, as well. Be sure to CHECK to make you don't write outside the buffer's length.

Originally Posted by Elysia

Any function that writes into a buffer provided as a parameter MUST have a size argument, as well.

Not if that is already accounted for in the flow of execution, which it is here. I don't believe Tool was trying to write a set of general purpose library functions.

Personally I would do that part (reading the line and allocating a buffer) somewhat differently, so as to not have to bother with FSEEK at all. However that would require strlen and strcpy so would probably be no more efficient.

Originally Posted by Bayint Naung

Reading twice to find out how many to allocate is not quite good.
Here is a good implementation.

That one is pretty lame IMO. It reallocs every 20 bytes, which is a complete waste of time. If you use a 1k stack buffer and fgets->strlen->malloc->strcpy you probably won't have to realloc at all, much less 3 or 4 times per line.

Originally Posted by MK27

Not if that is already accounted for in the flow of execution, which it is here. I don't believe Tool was trying to write a set of general purpose library functions.

That is irrelevant. It's a huge security risk nevertheless. To be sure, pass the real size of the buffer to the function and perform a security check. Don't rely on your logic to make sure it's correct. That will lead to loads and loads of bugs and a security nightmare.

That is irrelevant. It's a huge security risk nevertheless. To be sure, pass the real size of the buffer to the function and perform a security check. Don't rely on your logic to make sure it's correct. That will lead to loads and loads of bugs and a security nightmare.

You mean, instead of len variable, i should pass sizeof(buf) instead?

Yeah, good idea imo.

& btw, someone said they wouldnt use fseek.

Why?

However that would require strlen and strcpy so would probably be no more efficient.

Could you describe a bit more? Wouldnt this take much more code? Since you're using 2 more functions.

Also, if i'd use fgets, well. Then id have to check if the n - 1 character was a \n. Well, imo it would take more code then this way.

Is fseek "bad"?

Originally Posted by Elysia

That is irrelevant. It's a huge security risk nevertheless.

Malarky:

Code:

     int pos = 0;  
     while((alloc = next_line_length()) != EOF) {
         buf = (char *) malloc(alloc + 1); /* alloc + 1 for the \0 character */
         fseek(stdin, pos, SEEK_SET);      /* go back to start + pos positions in the file */
         getline(buf);                     /* store the line */
[....]
         pos += (alloc + 2);               /* increase pos by alloc + 2 (\n and next pos) */

That should be bulletproof. There is no security risk. If you find this confusing or uncertain that's your own flawed logic, not the code's.

Originally Posted by Tool

You mean, instead of len variable, i should pass sizeof(buf) instead?

You can't pass sizeof(buf), it's a pointer. If you want the (irrational) double saftey pass alloc.

Originally Posted by MK27

Malarky:

Code:

     int pos = 0;  
     while((alloc = next_line_length()) != EOF) {
         buf = (char *) malloc(alloc + 1); /* alloc + 1 for the \0 character */
         fseek(stdin, pos, SEEK_SET);      /* go back to start + pos positions in the file */
         getline(buf);                     /* store the line */
[....]
         pos += (alloc + 2);               /* increase pos by alloc + 2 (\n and next pos) */

That should be bulletproof. There is no security risk. If you find this confusing or uncertain that's your own flawed logic, not the code's.

This is a big security risk, because getline doesn't check to make it doesn't overwrite the buffer. Pass the size along, alloc + 1.

Originally Posted by MK27

You can't pass sizeof(buf), it's a pointer. If you want the (irrational) double saftey pass alloc.

Better safe than sorry.

@OP: You cannot use sizeof when you're using malloc. Furthermore, sizeof ONLY works in the function you created the original array in. Otherwise it will simply return the size of a pointer.