A pointer as a porameter of the function

[svata]

Hello there,

after some time of pondering I come to some solution which would suit
me best. Please correct, if I am wrong.
Function has two parameters. A string array, better said a pointer to
it, char *p_buf and int size.


int read_name( char *p_buf, int size) {
char *p_item_name_1;
char *p_item_name;

printf("Enter the description: ");
/* we want no more chracters than size!!! */
if (fgets(p_item_name_1, size, stdin) != NULL){
/* if the input contains a new line */
if (( p_item_name = strchr(p_item_name_1, '\n')) != NULL ){
*p_item_name = '\0'; /* get rid of new line, pointer p_item_name
points to it*/
}
else {
while(getchar() != '\n'){
;
}
}
strcpy(p_buf, p_item_name_1); /* and now copy it to the buffer
provided by caller */
return EXIT_SUCCESS;
}
else {
return EXIT_FAILURE;
}
}

in the main might be something like this:

char *p_new_buf;
char *buf;
int size = 10;
/* and here we can malloc buffer */
p_buf = malloc(sizeof(char) * size);

svata

 

[mark_bluemel]

Hello there,

after some time of pondering I come to some solution which would suit
me best. Please correct, if I am wrong.
Function has two parameters. A string array, better said a pointer to
it, char *p_buf and int size.


int read_name( char *p_buf, int size) {
char *p_item_name_1;
char *p_item_name;

Two poorly differentiated variable names...

printf("Enter the description: ");
/* we want no more chracters than size!!! */

Actually you probably want no more than (size -1), as you must _always_
allow for the trailing '\0'.

if (fgets(p_item_name_1, size, stdin) != NULL){

p_item_name_1 hasn't been initialised. You now read data into some
arbitrary place in memory.

/* if the input contains a new line */
if (( p_item_name = strchr(p_item_name_1, '\n')) != NULL ){

Why use that name for the address of the newline? It doesn't help
readability.

*p_item_name = '\0'; /* get rid of new line, pointer p_item_name
points to it*/
}
else {
while(getchar() != '\n'){
;
}

You still don't deal with EOF...

}
strcpy(p_buf, p_item_name_1); /* and now copy it to the buffer
provided by caller */

Why didn't you just read into there to start with?

return EXIT_SUCCESS;
}
else {
return EXIT_FAILURE;
}
}

in the main might be something like this:

char *p_new_buf;
char *buf;
int size = 10;
/* and here we can malloc buffer */
p_buf = malloc(sizeof(char) * size);

Why do you declare two char * items, p_new_buf and buf, but then assign
to p_buf?

(It's usually best to actually write and compile your code, then cut
and paste into the message you post to the newsgroup, then this sort of
nonsense can be avoided).

malloc() doesn't always succeed. You should develop the habit of
checking whether it did...

 

[santosh]

Hello there,

after some time of pondering I come to some solution which would suit
me best. Please correct, if I am wrong.
Function has two parameters. A string array, better said a pointer to
it, char *p_buf and int size.

It would be better if took some time to learn the basics before
attempting to code.

int read_name( char *p_buf, int size) {

size could be made const int.

char *p_item_name_1;
char *p_item_name;

printf("Enter the description: ");

Be aware that unless the output is terminated with a newline _or_ a
called to fflush() is made, the output may not appear when expected.

/* we want no more chracters than size!!! */
if (fgets(p_item_name_1, size, stdin) != NULL){

Where have you allocated space for this input? p_item_name_1 is
declared, but is not initialised to point a block of memory for holding
the input, niether is it set to NULL. The above statement will ensure
that fgets() tries to write to wherever p_item_name_1 happens to be
pointing, most likely resulting in undefined behaviour.

/* if the input contains a new line */
if (( p_item_name = strchr(p_item_name_1, '\n')) != NULL ){

You've used p_item_name_1 with fgets() above but here you use another
pointer, again uninitialised. More undefined behaviour.

*p_item_name = '\0'; /* get rid of new line, pointer p_item_name
points to it*/
}
else {
while(getchar() != '\n'){
;
}
}
strcpy(p_buf, p_item_name_1); /* and now copy it to the buffer
provided by caller */

Check wether the caller has indeed passed a non-null pointer, the
maximum pointer checking you can do within standard C.

return EXIT_SUCCESS;
}
else {
return EXIT_FAILURE;
}
}

in the main might be something like this:

char *p_new_buf;
char *buf;
int size = 10;
/* and here we can malloc buffer */
p_buf = malloc(sizeof(char) * size);

Firstly sizeof(char) is always one, so you need not include it.
Secondly always check the return value of malloc() and the like for
failure. If you get into the habit of checking the return value of all
functions for failure, it will be a lot more easier later on.

 

[svata]

So I rewrote it:

/* function to check malloc */
int malloc_check(char *in){
if (in == NULL ) {
printf("Failed to allocate memory on line %d", __LINE__);
return 1;
}
else {
return 0;
}
}

void read_name( char *p_buf, const int size) {
char *p_nl;
int ch;
int malloc_check(char *in);

printf("Enter the description: ");
fflush(stdout);
/* if there is some input */
if (fgets(p_buf, (size -1), stdin) != NULL){
/* malloc memory for p_nl ( one char ) */
p_nl = malloc(1); /* we need it to get rid of new line */
ch = malloc_check(p_nl); /* check malloc status */
if ( ch != 1 ){/* ie malloc returned success */
if (( p_nl = strchr(p_buf, '\n')) != NULL ){
*p_nl = '\0'; /* get rid of new line */
}
}
}
}

/* the only problem I have here, of at least I'm aware of, is that I
can't figure out how to correctly handle EOF in the context of thsi
function.
*/

and then in the main()

char *p_buf;
char *p_input;
void read_name(char *, const int);
int malloc_check(char *in);
int ch;

p_buf = malloc(10); /* malloc memory for p_buf */
ch = malloc_check(p_buf); /* check if it succeded */
if ( ch != 1 ) {/* if yes, do... */
read_name(p_buf, 10);
p_input = malloc(10);/* malloc memory for p_input */
if ( ch != 1 ) {/* if the case of success... */
strcpy(p_input, p_buf);/* copy content of p_buf to p_item */
free(p_buf);/* free memory*/
}
printf("%s", p_input);
}

Function works, I have no compile errors, but after entering item name
is "a new line inserted" and then if I press enter, then promp "Enter
the description appears". So I'm sure there is carriage return/new line
handled in bad way.

Other thing is, that I would like to store only the lenght of string I
wish( and drop the rest what exceeds the lenght ), but if I enter
longer one, nothing is stored at all. I suspect EOF not being handled
properly.

Hope, this code gets better response from you

svata

 

[marora]

So I rewrote it:

/* function to check malloc */
int malloc_check(char *in){
if (in == NULL ) {
printf("Failed to allocate memory on line %d", __LINE__);
return 1;
}
else {
return 0;
}
}

void read_name( char *p_buf, const int size) {
char *p_nl;
int ch;
int malloc_check(char *in);

1. you use malloc_check(...) but do not pass a valid parameter.
2. As said before this code will not compile, i will re-iterate, you
should compile your code first, before posting it here, atleast by
doing that you will have corrected errors such as these.
3. You return 1 or 0 from malloc_check, but you donot do anything with
that check.

printf("Enter the description: ");
fflush(stdout);
/* if there is some input */
if (fgets(p_buf, (size -1), stdin) != NULL){

Before using p_buff, you should confirm that p_buff isn't null.

/* malloc memory for p_nl ( one char ) */
p_nl = malloc(1); /* we need it to get rid of new line */

You should try to confirm if mallock returned you a null pointer here,
instead of doing it in a sperate function, it is not a good practise to
pass a NULL pointer to a function.

ch = malloc_check(p_nl); /* check malloc status */
if ( ch != 1 ){/* ie malloc returned success */
if (( p_nl = strchr(p_buf, '\n')) != NULL ){
*p_nl = '\0'; /* get rid of new line */
}
}
}

You need to free p_nl

}

/* the only problem I have here, of at least I'm aware of, is that I
can't figure out how to correctly handle EOF in the context of thsi
function.
*/

and then in the main()

char *p_buf;
char *p_input;
void read_name(char *, const int);
int malloc_check(char *in);
int ch;

p_buf = malloc(10); /* malloc memory for p_buf */
ch = malloc_check(p_buf); /* check if it succeded */
if ( ch != 1 ) {/* if yes, do... */
read_name(p_buf, 10);
p_input = malloc(10);/* malloc memory for p_input */
if ( ch != 1 ) {/* if the case of success... */
strcpy(p_input, p_buf);/* copy content of p_buf to p_item */
free(p_buf);/* free memory*/

You need to all of malloc'ed memory i.e. p_input

 

[santosh]

So I rewrote it:

gcc -Wall -Wextra -ansi -pedantic -o c c.c
c.c: In function 'malloc_check':
c.c:3: error: 'NULL' undeclared (first use in this function)
c.c:3: error: (Each undeclared identifier is reported only once
c.c:3: error: for each function it appears in.)
c.c:4: warning: implicit declaration of function 'printf'
c.c:4: warning: incompatible implicit declaration of built-in function
'printf'
c.c:11: warning: control reaches end of non-void function
c.c: In function 'read_name':
c.c:18: warning: incompatible implicit declaration of built-in function
'printf'
c.c:19: warning: implicit declaration of function 'fflush'
c.c:19: error: 'stdout' undeclared (first use in this function)
c.c:21: warning: implicit declaration of function 'fgets'
c.c:21: error: 'stdin' undeclared (first use in this function)
c.c:21: error: 'NULL' undeclared (first use in this function)
c.c:23: warning: implicit declaration of function 'malloc'
c.c:23: warning: incompatible implicit declaration of built-in function
'malloc'
c.c:26: warning: implicit declaration of function 'strchr'
c.c:26: warning: incompatible implicit declaration of built-in function
'strchr'
c.c: In function 'main':
c.c:47: warning: incompatible implicit declaration of built-in function
'malloc'
c.c:53: warning: implicit declaration of function 'strcpy'
c.c:53: warning: incompatible implicit declaration of built-in function
'strcpy'
c.c:54: warning: implicit declaration of function 'free'
c.c:56: warning: incompatible implicit declaration of built-in function
'printf'
sh-3.1#

How are you saying that it compiles fine? Are you cutting and pasting
the actual code?

 

[Chris Dollin]

So I rewrote it:

svata, /get a decent C book/ and /work through it/. Really.
(My personal choice would be K&R 2, but I believe the FAQ
has other choices.) You're getting more and more tied up
in detail before you've got the bigger picture clear.

Leaving aside the compilation problems others are telling
you about ...

/* function to check malloc */
int malloc_check(char *in){
if (in == NULL ) {
printf("Failed to allocate memory on line %d", __LINE__);
return 1;
}
else {
return 0;
}
}

You realise that __LINE__ will always be the same -- a line
number in `malloc_check` -- and so will not be useful if
it's used in more than one place?

Also it's not a very good abstraction. It's not coupled to
the malloc call it's supposed to be checking.

void read_name( char *p_buf, const int size) {
char *p_nl;
int ch;
int malloc_check(char *in);

/Do not do this/. It's legal but generally wrong: better is
to have such prototypes at file scope. In this instance,
it's also unnecessary, since `malloc_check` is already
declared above.

You should document that `p_buf` is not permitted to be
null.

printf("Enter the description: ");
fflush(stdout);
/* if there is some input */
if (fgets(p_buf, (size -1), stdin) != NULL){

Why `size - 1`? `fgets` knows that it has to reserve a character
for the terminating zero.

/* malloc memory for p_nl ( one char ) */
p_nl = malloc(1); /* we need it to get rid of new line */
ch = malloc_check(p_nl); /* check malloc status */
if ( ch != 1 ){/* ie malloc returned success */
if (( p_nl = strchr(p_buf, '\n')) != NULL ){
*p_nl = '\0'; /* get rid of new line */
}

That code is horribly wrong. Look, you don't need to mallocate
store for p_nl: you're going to get (or not) a pointer to the
newline (if any) inside `p_buf`. So the `malloc` is unnecessary,
and the `malloc_check` isn't necessary. What's more, if the
`malloc` /was/ useful, and it failed, there's no way this
important information can get out to the caller!

 

[svata]

gcc -Wall -Wextra -ansi -pedantic -o c c.c

How are you saying that it compiles fine? Are you cutting and pasting
the actual code?

I'm sorry, but I have no gcc here, so I'm unable to do -Wall and
pedantic check... It compiles here using Borland compiler and I know
there are some issues. I admit it.

svata

 

[santosh]

So I rewrote it:

More specific comments.

/* function to check malloc */
int malloc_check(char *in){

Again make the parameter const qualified, as the code in the function
need not modify it.

if (in == NULL ) {
printf("Failed to allocate memory on line %d", __LINE__);

This will always print the line number of the line the above printf()
is on, _not_, as you probably wanted, the line number of the failing
malloc() call.

return 1;
}
else {
return 0;
}
}

This whole function is a very poor design choice. It would be better,
atleast in small projects, to wrap malloc() and it's associated
debugging and error-handling code within a single function, say,
my_malloc().

void read_name( char *p_buf, const int size) {
char *p_nl;
int ch;
int malloc_check(char *in);

Although legal, don't place declarations within functions. Place them
outside of any function and before that function is called in the
source file. Generally, it's good practise to declare functions and
other objects right after the #include for header files and macro
definitions. In larger and multi-module projects, it would be better to
encapsulate them into a common header file and include that.

printf("Enter the description: ");
fflush(stdout);
/* if there is some input */
if (fgets(p_buf, (size -1), stdin) != NULL){
/* malloc memory for p_nl ( one char ) */
p_nl = malloc(1); /* we need it to get rid of new line */

This is not necessary.

ch = malloc_check(p_nl); /* check malloc status */
if ( ch != 1 ){/* ie malloc returned success */
if (( p_nl = strchr(p_buf, '\n')) != NULL ){
*p_nl = '\0'; /* get rid of new line */

This will cause a memory leak of 1 byte due to the unnecessary
allocation previously.

/* the only problem I have here, of at least I'm aware of, is that I
can't figure out how to correctly handle EOF in the context of thsi
function.
*/

and then in the main()

This is why you should cut and paste your exact and complete code.
Because you've only pasted a incomplete portion of main(), we will not
be able to compile it here, without encountering possibly spurious
errors.

char *p_buf;
char *p_input;
void read_name(char *, const int);
int malloc_check(char *in);

Again place the above function declarations outside of any functions
and preferably at the top of the source file. Also the declaration of
malloc_check above conflicts with it's definition previously.

int ch;

p_buf = malloc(10); /* malloc memory for p_buf */
ch = malloc_check(p_buf); /* check if it succeded */
if ( ch != 1 ) {/* if yes, do... */
read_name(p_buf, 10);
p_input = malloc(10);/* malloc memory for p_input */
if ( ch != 1 ) {/* if the case of success... */
strcpy(p_input, p_buf);/* copy content of p_buf to p_item */
free(p_buf);/* free memory*/
}
printf("%s", p_input);
}

All this is horribly convolvuted and wrong. I suggest that you work
your way up from simpler programs than this one. Also your
understanding of pointers and dynamic memory are very shaky. Please go
through a good textbook.

Hope, this code gets better response from you

You're implementing before carefully thinking out your program on pen
and paper and before understanding many of the fundamentals of C. As
such it would be better to scrap this program and start with a simpler
one. Use static arrays to begin with, progressing to dynamic memory
when you've got all other issues ironed out.

Don't be discouraged by my comments. Most newbies will have some
trouble early on. Practise and re-reading will usually improve matters
dramatically.

 

[svata]

That code is horribly wrong. Look, you don't need to mallocate
store for p_nl: you're going to get (or not) a pointer to the
newline (if any) inside `p_buf`. So the `malloc` is unnecessary,
and the `malloc_check` isn't necessary. What's more, if the
`malloc` /was/ useful, and it failed, there's no way this
important information can get out to the caller!

So is there any possibility to write any wrapper of macro around malloc
to handle that in proper way?

svata

 

[santosh]

So is there any possibility to write any wrapper of macro around malloc
to handle that in proper way?

In that particular instance you attempt to allocate one byte with a
call to malloc(), and assign the return value to p_nl. strchr() however
returns a value of type pointer to char, (or NULL if it fails in it's
search). You overwrite the previous value of p_nl, (that you got from
the malloc() call), with this new value, thus losing access to the
single byte you allocated previously, causing a memory leak, (i.e.
memory that can niether be used nor be returned to the system).

Now, coming to the wrapper for malloc(), there're any number of ways to
go about, depending on the program's requirements and your skill level,
but at a minimum try translating the following pseudo-code to C.

void *my_malloc(const size_t blksize, const int calling_line) {
CHECK_ARGS;
IF malloc(blksize) FAILS
PRINT (ERROR_MSG: LINE NUMBER: calling_line);
EXIT_OR_WHATEVER;
ELSE
RETURN POINTER;
}

As you progress, you'll find that it's convinient to construct wrappers
of varying complexity around many of the standard C library's
functions, to better encapsulate error handling.

 

[svata]

This whole function is a very poor design choice. It would be better,
atleast in small projects, to wrap malloc() and it's associated
debugging and error-handling code within a single function, say,
my_malloc().

Yes, so how to design it?

Although legal, don't place declarations within functions. Place them
outside of any function and before that function is called in the
source file. Generally, it's good practise to declare functions and
other objects right after the #include for header files and macro
definitions. In larger and multi-module projects, it would be better to
encapsulate them into a common header file and include that.

Yes, I did because I placed my functions in wrong order...

This will cause a memory leak of 1 byte due to the unnecessary
allocation previously.

Ok, now I know it

This is why you should cut and paste your exact and complete code.
Because you've only pasted a incomplete portion of main(), we will not
be able to compile it here, without encountering possibly spurious
errors.

I removed as much as possible of unnecessary stuff, but I get no
errors.

All this is horribly convolvuted and wrong. I suggest that you work
your way up from simpler programs than this one. Also your
understanding of pointers and dynamic memory are very shaky. Please go
through a good textbook.

You might be right, I admit it is not the easy one. But I like to learn
by doing.
I know that my understanding might be too shaky, but I assume that I
will learn more as I do more coding. Thus my code might be more steady.

You're implementing before carefully thinking out your program on pen
and paper and before understanding many of the fundamentals of C. As
such it would be better to scrap this program and start with a simpler
one. Use static arrays to begin with, progressing to dynamic memory
when you've got all other issues ironed out.

No, in my humble opinion not. I did similar one with static
structure... so next step is to rewrite it to use dynamic memory
allocation.

Don't be discouraged by my comments. Most newbies will have some
trouble early on. Practise and re-reading will usually improve matters
dramatically.

No, I don't feel discouraged, it's very useful to have some feedback,
it's best way to learn something new and improve my skill.

svata

 

[mark_bluemel]

svata, /get a decent C book/ and /work through it/. Really.
(My personal choice would be K&R 2, but I believe the FAQ
has other choices.) You're getting more and more tied up
in detail before you've got the bigger picture clear.

And once you've done the basic C book, try reading something on design
- a number of my colleagues have found "Code Complete" helpful.

 

[Chris Dollin]

So is there any possibility to write any wrapper of macro around malloc
to handle that in proper way?

Why a macro?

It depends on what "the proper way" is, and that's application-dependant.

In the case of p_nl, you didn't need to use `malloc` /at all/.

 

[Chris Dollin]

santosh wrote:

Yes, I did because I placed my functions in wrong order...

Doesn't matter: you still don't need to (and shouldn't) put the
declarations /inside/ the function.

 

[svata]

Doesn't matter: you still don't need to (and shouldn't) put the
declarations /inside/ the function.

But then all variables will be declared as global variables.

svata

 

[Arthur J. O'Dwyer]

But then all variables will be declared as global variables.

Chris originally said, re:

Although legal, don't place declarations within functions. Place them
outside of any function and before that function is called in the
source file.

He was talking about /function/ declarations, such as 'malloc_check'
in the given code sample. Nobody was ever talking about /variable/
/definitions/, which obviously should go wherever you want to define
those variables.

The same "don't declare things in functions" rule goes for variables,
too, by the way; I would studiously avoid any code that tried to declare

int main(void)
{
extern int foo, bar;
[...]
}

/Definitions/ are a different matter.

HTH,
-Arthur

 

[Keith Thompson]

But then all variables will be declared as global variables.

Chris originally said, re:

Although legal, don't place declarations within functions. Place them
outside of any function and before that function is called in the
source file.

He was talking about /function/ declarations, such as 'malloc_check'
in the given code sample. Nobody was ever talking about /variable/
/definitions/, which obviously should go wherever you want to define
those variables.

The same "don't declare things in functions" rule goes for variables,
too, by the way; I would studiously avoid any code that tried to declare

int main(void)
{
extern int foo, bar;
[...]
}

/Definitions/ are a different matter.

Just to add to the frivolity, all definitions are also declarations.
So this:
extern int foo;
is a declaration but not a definition, but this:
int foo;
is both a declaration and a definition.

Think of it this way: A declaration declares that something exists,
but it may or may not *create* it. A definition declares that
something exists *and* it creates it.

Variable definitions within function bodies are perfectly acceptable.
Variable declarations that aren't also definitions are legal, but
discouraged.

 

[J. J. Farrell]

You should try to confirm if mallock returned you a null pointer here,
instead of doing it in a sperate function, it is not a good practise to
pass a NULL pointer to a function.

Why do you think it is not a good practice to pass a null pointer to a
function?

 

[Chris Dollin]

But then all variables will be declared as global variables.

The declarations /of the functions/, which is what I was referring
to. santosh had said, referring to your:

| int malloc_check(char *in);

this:

| Although legal, don't place declarations within functions. Place them
| outside of any function and before that function is called in the
| source file. Generally, it's good practise to declare functions and
| other objects right after the #include for header files and macro
| definitions. In larger and multi-module projects, it would be better to
| encapsulate them into a common header file and include that.

And you replied:

| Yes, I did because I placed my functions in wrong order...

to which I said:

Looking back it's probably wasn't immediately clear that it
was specifically declarations /of functions/ that santosh was
talking about.