character array manipulation - coding style

[amer.terzic]

Here's what my code is supposed to do (it seems to work fine)

Take a char* array and remove a first character from it....as simple as
that.

The mentioned char* array is a 'global' var shared among threads.

Declared as : char* buffer = (char*) malloc(bufferSize)
(later I changed it to use C++ 'new', but it does the same thing)


Then I have a function declared as :

void* readBuffer(void*)
{
blah blah

char* copy=malloc and stuff ;

copy=buffer; // would YOU do it this way ?

blah (do stuff with copy )

*buffer++; /* so now buffer lost its first element
at buffer[0] */
}


Here's my question:

How bad is this code when it comes to style?

Does it make sense to do *buffer++ to cut the first character
off ?

It works just fine, but I'd like to know if something like this would
be acceptable in the real world ( as opposed to school assignment)?

Should I 'clean up' after myself (buffer[0] from before now sits in
memory unused) ?

How would I clean up after myself ?

The reason I did not use strncpy or anyother 'built-in' function is due
to restrictions in the problem itself.

I'd appreciate a comment.

 

[Richard Heathfield]

(e-mail address removed) said:

Here's what my code is supposed to do (it seems to work fine)

Take a char* array and remove a first character from it....as simple as
that.

The mentioned char* array is a 'global' var shared among threads.

Declared as : char* buffer = (char*) malloc(bufferSize)

Drop the cast.

(later I changed it to use C++ 'new', but it does the same thing)

No, it doesn't. Decide whether you are using C or C++. If you are using C++,
you're in the wrong place. If you want C, new doesn't work like you might
expect it to.

Then I have a function declared as :

void* readBuffer(void*)
{
blah blah

char* copy=malloc and stuff ;

copy=buffer; // would YOU do it this way ?

No. I'd write something that compiles.

blah (do stuff with copy )

*buffer++; /* so now buffer lost its first element
at buffer[0] */

The leading * is meaningless.

}


Here's my question:

How bad is this code when it comes to style?

It's appalling. Ask your compiler, and it will agree.

Does it make sense to do *buffer++ to cut the first character
off ?
No.

It works just fine, but I'd like to know if something like this would
be acceptable in the real world ( as opposed to school assignment)?

You'd be astounded at what's considered acceptable in the real world. But if
I were your tech lead and you showed me this, I'd get you onto an in-house
C course as soon as possible.

Should I 'clean up' after myself (buffer[0] from before now sits in
memory unused) ?

No, just don't use that byte in the first place! There's no need. You have
total control over the allocation and copying, right? So just Don't Copy
That Byte. Start at the next byte.

size_t len = strlen(input); /* dropping first byte - "ABC" -> "BC" */
char *new = malloc(len);
if(new != NULL)
{
memcpy(new, input + 1, len);
}

How would I clean up after myself ?

When you are done with the memory, call the free() function, passing it the
pointer value returned by malloc.

 

[Christopher Layne]

The mentioned char* array is a 'global' var shared among threads.

Sounding like a bad idea already, but alright.

Declared as : char* buffer = (char*) malloc(bufferSize)
(later I changed it to use C++ 'new', but it does the same thing)

Then I have a function declared as :

void* readBuffer(void*)
{
blah blah

char* copy=malloc and stuff ;

copy=buffer; // would YOU do it this way ?

No, because I don't like memory leaks.

blah (do stuff with copy )

*buffer++; /* so now buffer lost its first element
at buffer[0] */
}

You mean buffer++, although *buffer++ probably wouldn't hurt in this context -
but it's misleading and pointless.

 

[Frederick Gotham]

Amer posted:

char* copy=malloc and stuff ;

copy=buffer; // would YOU do it this way ?

I think we should make a Pointers Tutorial and add it to the FAQ -- there are
several posters each day who simply don't understand how pointers work.

 

[T.M. Sommers]

Here's what my code is supposed to do (it seems to work fine)

Take a char* array and remove a first character from it....as simple as
that.

The mentioned char* array is a 'global' var shared among threads.

Declared as : char* buffer = (char*) malloc(bufferSize)
(later I changed it to use C++ 'new', but it does the same thing)


Then I have a function declared as :

void* readBuffer(void*)
{
blah blah

char* copy=malloc and stuff ;

copy=buffer; // would YOU do it this way ?

You just leaked the memory allocated with malloc.

blah (do stuff with copy )

*buffer++; /* so now buffer lost its first element
at buffer[0] */
}


Here's my question:

How bad is this code when it comes to style?
Ghastly.

Does it make sense to do *buffer++ to cut the first character
off ?

No. For one thing, there is no point in dereferencing it. For
another, it leaks the first character. For yet another, if you
ever try to free() it, you will get undefined behavior.

It works just fine,

It doesn't.

but I'd like to know if something like this would
be acceptable in the real world ( as opposed to school assignment)?

It isn't.

Should I 'clean up' after myself (buffer[0] from before now sits in
memory unused) ?

How would I clean up after myself ?

The most straightforward way to solve the problem is just to
shift everything in the array one position to the left (or down,
whichever way you want to look at it). Use memmove() or do it
yourself.

The reason I did not use strncpy or anyother 'built-in' function is due
to restrictions in the problem itself.

Then do your own version of strncpy(). Regardless of
restrictions in your problem, if you want a copy, you have to
make a copy, one way or another.

 

[Nelu]

Here's what my code is supposed to do (it seems to work fine)

Take a char* array and remove a first character from it....as simple as
that.

The mentioned char* array is a 'global' var shared among threads.

C doesn't know about threads.

Declared as : char* buffer = (char*) malloc(bufferSize)
(later I changed it to use C++ 'new', but it does the same thing)

Supposing you want to stick with C (otherwise this is not the
right group) forget about *new* and don't use the (char *) cast.
It can mask the fact that you may have forgotten to include stdlib.

Then I have a function declared as :

void* readBuffer(void*)

Are you using the argument? If yes, you should have given its name.

{
blah blah

Is that a macro?

char* copy=malloc and stuff ;

Here you allocate memory for copy.

copy=buffer; // would YOU do it this way ?

Here you make the value of copy equal the value of buffer. This
means that copy and buffer point to the same thing that buffer
used to point to. Unless stuff (from above) is a list of
instructions that either tell buffer to point to copy or make
another pointer point to the memory allocated for copy you have
lost the address of the allocated memory (memory leak).
Whether I would or wouldn't do it that way depends on what I need
to do. Also, remember the memory you lost and can't find anymore.

blah (do stuff with copy )

Whatever you do to change things in copy will affect buffer in
the same way.

*buffer++; /* so now buffer lost its first element
at buffer[0] */

Make sure buffer has something in it... i.e. it's not NULL and it
has enough allocated memory to allow you to do that.

}


Here's my question:

How bad is this code when it comes to style?

It's wrong.

Does it make sense to do *buffer++ to cut the first character
off ?

You don't cut it off. It's still there, still allocated. It's
just that you ignore it and move away from it. You will still
need to free the memory starting at the original position.

It works just fine, but I'd like to know if something like this would
be acceptable in the real world ( as opposed to school assignment)?

You have memory leaks and other problems. What do you think?

Should I 'clean up' after myself (buffer[0] from before now sits in
memory unused) ?

Yes, you should free resources when you know you no longer use
them. copy's initially allocated memory stays unused and you
can't clean it up because you lost it's address when you did
copy=buffer.

How would I clean up after myself ?

free(<address>)
<OT>
After you make sure there's no other thread using the shared
resource...

The reason I did not use strncpy or anyother 'built-in' function is due
to restrictions in the problem itself.

does *for* count for built-in or can you use it to do the copy?

I'd appreciate a comment.

Post code that compiles. Things that you leave out may be of
interest even if you may not realize it.

 

[amer.terzic]

Thank you everyone.

I appreciate the help.

I knew the code was a mess and it r e a l l y bothered me
because I did not know what was wrong with it.

My goal is to learn from mistakes BEFORE someone hires me and lets me
code 'cause I know the professors here will never yell at me if I use
the code above.

Some day I'll sit down and properly learn how to use pointers, as
opposed to current mind set, which is 'if it compiles - it must be
good' .

(I'm off to put a band-aid on my hurting pride ...lol)


Thanks again.

 

[Christopher Layne]

Some day I'll sit down and properly learn how to use pointers, as
opposed to current mind set, which is 'if it compiles - it must be
good' .

(I'm off to put a band-aid on my hurting pride ...lol)


Thanks again.

If you're using gcc, I suggest you do the following when compiling your own
code:

export CFLAGS="-ansi -pedantic -Wall -W"
export CXXFLAGS="-ansi -pedantic -Wall -W"

or bastard mode:

export CFLAGS="-ansi -pedantic -pedantic-errors -Wall -W -Werror"
export CXXFLAGS="-ansi -pedantic -pedantic-errors -Wall -W -Werror"

 

[Snis Pilbor]

Thank you everyone.

I appreciate the help.

I knew the code was a mess and it r e a l l y bothered me
because I did not know what was wrong with it.

My goal is to learn from mistakes BEFORE someone hires me and lets me
code 'cause I know the professors here will never yell at me if I use
the code above.

Some day I'll sit down and properly learn how to use pointers, as
opposed to current mind set, which is 'if it compiles - it must be
good' .

(I'm off to put a band-aid on my hurting pride ...lol)


Thanks again.

Where are you going to school? The code you posted, unless there was
some pretty radical code hidden behind your "and stuff" lines,
indicates you don't have the foggiest notion how pointers work. If
your professors don't raise eyebrows about that, they are incompetent
to teach and need to be shifted to pure research/admin positions (or
fired if they're just lecturers)

Think of pointers as just a fancy kind of number: the number is the
address of a block of memory, which you may or may not be allowed to
look at/write to. It's honestly really that simple, it's no more
complicated than learning to use ints. In the code you posted, you
have this fancy number variable, and you store the value of a malloc to
it. The malloc function does dirty work behind the scenes to find a
block of RAM that you are allowed to read/edit, and returns its
address. You store that address in your fancy variable, but a few
lines down, overwrite it with "buffer" (whatever the heck that is).
Which means now that block of RAM is "leaked": you have no way to tell
the computer "I'm done with that block of RAM, it can be used for other
things now", so if your program runs long enough and calls that
function often enough, it'll suck up every ounce of RAM your computer
has.

 

[Michael Mair]

If you're using gcc, I suggest you do the following when compiling your own
code:

export CFLAGS="-ansi -pedantic -Wall -W"
export CXXFLAGS="-ansi -pedantic -Wall -W"

or bastard mode:

export CFLAGS="-ansi -pedantic -pedantic-errors -Wall -W -Werror"
export CXXFLAGS="-ansi -pedantic -pedantic-errors -Wall -W -Werror"

Note that, apart from the above being a shell-specific solution
to set C*FLAGS, -W in newer gcc versions is called -Wextra.
I'd rather include -O instead of -W, though, if it comes to sheer
usefulness of the warnings and work with splint on the rest.

Cheers
Michael

 

[amer.terzic]

Where are you going to school?

University of Buffalo ....NY ....USA

The code you posted, unless there was
some pretty radical code hidden behind your "and stuff" lines,
indicates you don't have the foggiest notion how pointers work.

Nope, no radical code.

If your professors don't raise eyebrows about that, they are incompetent
to teach and need to be shifted to pure research/admin positions (or
fired if they're just lecturers)

I know. All the school cares about is research.

Think of pointers as just a fancy kind of number: the number is the
address of a block of memory, which you may or may not be allowed to
look at/write to.

That I know and I have no excuse for abusing pointers the way I did.

It's honestly really that simple, it's no more
complicated than learning to use ints. In the code you posted, you
have this fancy number variable, and you store the value of a malloc to
it.
The malloc function does dirty work behind the scenes to find a
block of RAM that you are allowed to read/edit, and returns its
address. You store that address in your fancy variable, but a few
lines down, overwrite it with "buffer" (whatever the heck that is).
Which means now that block of RAM is "leaked": you have no way to tell
the computer "I'm done with that block of RAM, it can be used for other
things now", so if your program runs long enough and calls that
function often enough, it'll suck up every ounce of RAM your computer
has.

The sad thing is that my peers know even less about pointers.
But nonetheless they will proudly put 'knowledge of C' on their resume
and get a job where they need to apply it.
Ooops, this is not the place to discuss t h a t.

btw. thanks for the compiler flags....

 

[Nelu]

University of Buffalo ....NY ....USA


Nope, no radical code.

Please do not snip attribution lines. Let people know who wrote
what, otherwise it will be difficult to find out who you are
replying to... also, some people may get angry and sue you

 

[Keith Thompson]

Think of pointers as just a fancy kind of number: the number is the
address of a block of memory, which you may or may not be allowed to
look at/write to. It's honestly really that simple, it's no more
complicated than learning to use ints.

Actually, it's usually best *not* to think of pointers as numbers
(unless you have a very expansive concept of what a "number" is).

Pointers are pointers. Pointers are not numbers. A pointer indicates
the location of something; the manner in which it does so is
irrelevant.

On many (most?) implementations, a pointer *is* represented as an
unsigned integer, representing an address in virtual memory, but
that's not required by the standard. You can perform limited kinds of
arithmetic on pointers, but that's not the same as integer arithmetic;
it's pointer arithmetic, and needs to be understood on its own terms.

You might find it useful to think of pointers as similar to URLs. A
URL is a string of characters; you don't care about how it's
constructed, merely that you can use it to refer to something.
There's no ordering to them, and no relationship other than inequality
between URLs that point to different sites (what is "http://cnn.com" -
"http://slashdot.org"?). There is some structure to how URLs are
constructed, but you don't need to know about that to surf the web.

Similarly, you shouldn't be concerned about how pointers are
represented. They point to things, and you can perform a limited set
of operations on them. And there definitely have been real-world
systems in which pointers happen to have internal structure beyond
just being an integer index into virtual memory.

If you want to learn about the system-specific details, that's great,
and it will be helpful to understand how things can work "under the
hood" -- but you shouldn't usually depend on this when writing
ordinary C code, any more than you need to know whether you're running
on an x86 or a PowerPC when you write a "hello, world" program.