Thread: Incorrect format using printf()

I suspected a newline and a space being stuck in the first position of workers.lname. I used the strlen() function to count the number of chars after both fname and lname, and counting the '\0' I get the exact number as expected.

fgets does include a newline at the end of the string. And the format of your printf: "\n\n%s %s" (I bolded the space but you can't see that ) accounts for the leading space in the second line.

EDIT: But don't get rid of the space; I was just mentioning that part.
And strlen doesn't count the \0.

And all
scanf("%u", workers[count].health);
should be
scanf("%u", &workers[count].health);

My program after modifications ran perfectly until I decided to upgrade it with the use
of a print_records function. If I'm able to fix that, I'd like to pull the code out of main that
creates the records into another function.

When I create the pointer to a type struct, I'm using a very similar example from
"Teach yourself C in 21 Days". 6th ed. 2004.

Has that much changed in the C world since 2004? Also, this stupid book near the last
chapter gave a 1 page example of what a bit field looks like inside a structure. I figured out
by myself how to us it.

The error that I'm getting is: Error 1 error C2040: 'pemp' : 'int' differs in levels of indirection from 'emp_data *'

I think I'm really close. I'm finishing this book tonight; I have the latest C Primer Plus next
to me that I've been drooling over the past 3 weeks. Using the teach yourself C in 21 has
been a love/hate relationship. More hate than love. I'll probably use it for kindling at tomorrow's barbecue.

The problem area is highlighted in red. Thanks for help!

Code:

//		9/14/10
//
// practice using bit fields inside structures
// while at the same time using Cprogramming.com approved I/O functions.
//

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

#define YES 1;
#define NO 0;
#define MAX 10  // set upper limit for # records here.

struct emp_data
{
		unsigned char	
			health	  : 2,	  // width for 4 possible types of health coverage
			dental	  : 1,
			college	  : 1,
			hen_house : 1;	// turns the light on in the chicken coop
	char fname[20];
	char lname[20];
	char ssnumber[12];
};

struct emp_data workers[MAX];

struct emp_data *pemp;	// create a pointer to type emp_data

pemp = workers;	// Initialize the pointer to workers first struct array[0]

void print_records( struct emp_data *qptr, int );


int main( void )
{
    void *ptr;	// declaring a generic pointer 

	

	unsigned bit;
	int count;

	for( count=0; count<MAX; count++ )
	{
		workers[count].dental = NO;	// initialize all bit fields
		workers[count].college = NO;
		workers[count].health = NO;

		printf("\nEnter first name: ");

		ptr = fgets(workers[count].fname, sizeof(workers[count].fname), stdin);
		if ( ptr == NULL )
		{
			printf("Error entering fname\n\n");	// Funtionality tested OK when entered cntrl^z
			return 1;
		}

		printf("Enter last name: ");

		ptr = fgets(workers[count].lname, sizeof(workers[count].lname), stdin);
		if ( ptr == NULL )
		{
			printf("Error entering lname\n\n");	// Abort if error
			return 1;
		}

		printf("\nDental?");

		scanf("%u", &bit );
		if( bit == 1 )
		{
			workers[count].dental = YES;
		}
	
		printf("\nCollege?");

		scanf("%u", &bit );
		if( bit == 1 )
		{
			workers[count].college = YES;
		}

			while (getchar() != '\n');	// <--- why did I have to resort to this??

			printf("\nHealth?");
			scanf("%u", &bit );		// toggle the bit fields

			switch ( bit)
			{
			case 0:
				{
					workers[count].health |= bit;
					break;
				}
			case 1:
				{
					workers[count].health |= bit;
					break;
				}
			case 2:
				{
					workers[count].health |= bit;
					break;
				}
			case 3:
				{
					workers[count].health |= bit;
					break;
				}
			default:
				{
					printf("Invalid entry for health\n");
					break;
				}
			}
			
		printf("\nEnter SS Number:\n");

		while (getchar() != '\n');	// clear stdin - Hammer's code from Prelude's corner

		ptr = fgets(workers[count].ssnumber, sizeof(workers[count].ssnumber), stdin);
		if ( ptr == NULL )
		{
			printf("Error entering ssnumber\n\n");	// Abort if error
			return 1;
		}

		while (getchar() != '\n');	// clear stdin
	}

	print_records( pemp, MAX );  // print out all records
     return 0;
}

void print_records( struct emp_data *sptr, int size )
{
	int count;

	for( count=0; count<size; count++ )	// print out the records entered
		{
			printf("\n\n%s%s", workers[count].fname, workers[count].lname);	// will use sptr when repairs are made

			printf("dental= %d\tCollege= %d\tHealth= %d\tSS Number= %s\n\n", 
				workers[count].dental, workers[count].college, workers[count].health, workers[count].ssnumber);

			sptr++;  // point to the next structure

			printf("\n");
		}
}

C changed much? You have to be kidding. C moves at a snail's pace. The next standard will literally bring next to nothing new to C after over 10 years of work.
Anyway, the problem is that you're assigning a pointer outside main. You can't do that. You must initialize it. That means you put "=" after the declaration and a valid value after.

Separate initialization and assignment.

void* p = NULL; // Initialization

void* p;
p = NULL; // Assignment

Initialization can be done outside main; assignment cannot.

Originally Posted by Char*Pntr

Oh, and if you think this Teach yourself C in 21 days is a good book, I'll be happy to give it to you!

That's a joke, right?

My comments here are mostly matters of style, they won't help you too much. You may want to just read over them, consider them, and possibly implement them next time.

I'm not too clear on what you're trying to do with that switch statement. I'm pretty sure that whatever it is, there's an easier way. All of the cases seem to be identical, so I'd like to point out in case you were unaware of it that you can also do this:

Code:

            switch ( bit)    // determine which health bits to set
            {
            case 0:
            case 1:
            case 2:
            case 3:
                  {
                    qptr->health |= bit;
                    break;
                  }
            default:
                  {
                    printf("Invalid entry for health\n");
                  }
            }

In other words, since the code continues executing until it hits a breakpoint, you can use it to your advantage to combine switch cases together.

Another comment: you use quite a few anonymous blocks in your code (it's something I do a lot of as well ), which can be good but in your case it sometimes is just a way to separate out some code that should really be put into a function. For example, both of these would make good functions.

Code:

        do 
        {
            printf("\nEnter Dental 1 or 0\n");    // toggle the 1-bit fields
            scanf("%u", &bit );
            if( bit == 1 )
                qptr->dental = YES;    
        } while ( bit > 1 );

Code:

        {    
            printf("\nEnter first name: ");

            ptr = fgets(buf, BUFSIZ, stdin);    // enters string from stdin
            if ( ptr == NULL )        // check for error
            {
                printf("Error entering fname\n\n");    // Funtionality tested OK when entered cntrl^z
                return 1;
            }
            if ((p = strchr(buf, '\n')) != NULL)  // get rid of that '\n' from the FAQ
                *p = '\0';    
            strncpy( qptr->fname, buf, name_max - 1 );    // only copys name_max-1 chars to destination from buf.
        }

Yes there is some code that is different, but you can easily pass in enough information to make a generic solution.

Lastly, fgets() actually returns a pointer of type char* (see the man page) -- although if you don't care about the type assigning it to a void* pointer is as good as anything. But your use of ptr could easily be combined into one line:

Code:

                if ( fgets(buf, BUFSIZ, stdin) == NULL )

Personally I find that easier to read, but again that's just a matter of style.

Be very careful with your read_string() function. There's a bug in it. If the read was successful, you return str, which is really buf since that's what fgets() returns on success. But buf is a local variable allocated on the stack, so when you use the returned value from read_string() you'll be accessing a variable that's out of scope. There's an interesting discussion of out-of-scope variables going on at the moment over in the C++ board: Where do they go?

The main issue: the data is still there, sitting on the stack, which is why the code seems to work; but because it's gone out of scope, you're officially not supposed to be accessing it anymore, and the operating system could reclaim it at any time. Furthermore, if you call another function it's more than likely to start overwriting some of the data on the stack and messing up your string.

Basically, you have three options for a read_string() function. You can:

1. Make buf a static variable. This is the easiest fix but it's a really bad idea, because if you call read_string() multiple times it will start overwriting the same buffer, which can lead to some nasty bugs.
2. Declare buf[] outside read_string() and instead pass it in as a parameter. This ensures that buf will continue to exist after read_string() returns. I would recommend this solution.
3. malloc() some memory in read_string(), put fgets()'s data into that buffer, and then return the malloc()'d memory. This is a somewhat more advanced option; it comes with some trickiness, since you have to remember to free() the data eventually (or else you'll have a memory leak). It's probably what I would use but if you're just starting out with functions I'd go with option 2.

BTW, if you go with the last option and you're feeling adventurous you can write a read_string() function that doesn't have a cap on the maximum length of a line that the user enters. Just combine realloc() with fgets() in a loop . . . .

Have fun.

Originally Posted by dwks

Be very careful with your read_string() function. There's a bug in it. If the read was successful, you return str, which is really buf since that's what fgets() returns on success. But buf is a local variable allocated on the stack, so when you use the returned value from read_string() you'll be accessing a variable that's out of scope. There's an interesting discussion of out-of-scope variables going on at the moment over in the C++ board: Where do they go?

thank you very much dwks, I will read up on this issue.

[*]Declare buf[] outside read_string() and instead pass it in as a parameter. This ensures that buf will continue to exist after read_string() returns. I would recommend this solution.

Actually I thought of this, but did not implement it that way, because I wanted to write
my function with no arguments, that simply returns a pointer. The implementation looked sleek.

[*]malloc() some memory in read_string(), put fgets()'s data into that buffer, and then return the malloc()'d memory. This is a somewhat more advanced option; it comes with some trickiness, since you have to remember to free() the data eventually (or else you'll have a memory leak). It's probably what I would use but if you're just starting out with functions I'd go with option 2.

Your 3rd suggestion is the one I'd like to do. Why? because it's more challenging.

BTW, if you go with the last option and you're feeling adventurous you can write a read_string() function that doesn't have a cap on the maximum length of a line that the user enters. Just combine realloc() with fgets() in a loop . . . .

I'll keep this in mind! However, I couldn't imagine a situation where the string entered from the keyboard would need to be so large. At this point, FILE *fp operations would probably be more suitable.

Again, the most important thing is for me to be able to replace scanf() when reading strings, in my code with my own solution. That way, in the future when I ask for help here for some problem, the problem at hand will be easily addressed, and I wont have to waste other peoples time with scanf(), gets() etc. issues.

Again I sure appreciate all your help. :-)

Edit:
This thread is complete. Please feel free to IM me for other comments. :-)